Redirect to the prefered console UI when the server root accessed#4499
Redirect to the prefered console UI when the server root accessed#4499mpmadhavig merged 2 commits intowso2:4.12.xfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new RootRedirectServlet that handles requests to "/" and issues an HTTP redirect to a configured default UI context or falls back to the default management UI path. The servlet is registered via the HTTP Whiteboard in CarbonUIServiceComponent. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| * @param carbonUIDefinitions the UI definitions containing context configuration | ||
| */ | ||
| public RootRedirectServlet(CarbonUIDefinitions carbonUIDefinitions) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| * @param carbonUIDefinitions the UI definitions containing context configuration | |
| */ | |
| public RootRedirectServlet(CarbonUIDefinitions carbonUIDefinitions) { | |
| public RootRedirectServlet(CarbonUIDefinitions carbonUIDefinitions) { | |
| this.carbonUIDefinitions = carbonUIDefinitions; | |
| log.info("RootRedirectServlet initialized with CarbonUIDefinitions"); | |
| } |
| private void handleRedirect(HttpServletRequest request, HttpServletResponse response) | ||
| throws IOException { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| private void handleRedirect(HttpServletRequest request, HttpServletResponse response) | |
| throws IOException { | |
| String redirectUrl = getRedirectUrl(request); | |
| log.info("Redirecting root path request to: " + redirectUrl); |
| // Register RootRedirectServlet to handle requests to "/" path | ||
| // This is necessary because without a servlet registered for "/", requests to the root path | ||
| // would result in a 404 error being forwarded to the error page instead of being handled | ||
| // by handleSecurity in CarbonSecuredHttpContext | ||
| Servlet rootRedirectServlet = new RootRedirectServlet(carbonUIDefinitions); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| // Register RootRedirectServlet to handle requests to "/" path | |
| // This is necessary because without a servlet registered for "/", requests to the root path | |
| // would result in a 404 error being forwarded to the error page instead of being handled | |
| // by handleSecurity in CarbonSecuredHttpContext | |
| Servlet rootRedirectServlet = new RootRedirectServlet(carbonUIDefinitions); | |
| // Register RootRedirectServlet to handle requests to "/" path | |
| // This is necessary because without a servlet registered for "/", requests to the root path | |
| // would result in a 404 error being forwarded to the error page instead of being handled | |
| // by handleSecurity in CarbonSecuredHttpContext | |
| log.info("Registering RootRedirectServlet for root path handling"); | |
| Servlet rootRedirectServlet = new RootRedirectServlet(carbonUIDefinitions); |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 |
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/22173714253
madurangasiriwardena
force-pushed
the
root-path
branch
2 times, most recently
from
a750f9b to
0c62083
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java`:
- Around line 257-266: The RootRedirectServlet is registered with the pattern
"/" which acts as a default handler for unmatched paths; update the registration
in CarbonUIServiceComponent or the servlet to ensure only the exact root is
handled: either change the servlet registration pattern passed to
context.registerService(Servlet.class, rootRedirectServlet,
rootRedirectServletProps) from "/" to an empty pattern "" for exact context-root
matching, or add a root-only guard inside RootRedirectServlet (e.g., in
handleRedirect() / doGet()) that checks the request path
(request.getRequestURI() and/or request.getContextPath()) and only performs the
redirect when the path is exactly the context root ("/" or empty), otherwise
respond with 404/chain to next handler.
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java`:
- Around line 102-104: The code in RootRedirectServlet dereferences
carbonUIDefinitions.getContexts() without checking for null which can cause a
500; update the conditional around carbonUIDefinitions to also check that
carbonUIDefinitions.getContexts() != null before calling containsKey/get, e.g.
guard the expression using carbonUIDefinitions != null &&
carbonUIDefinitions.getContexts() != null &&
carbonUIDefinitions.getContexts().containsKey("default-context") and then set
defaultContext from carbonUIDefinitions.getContexts().get("default-context").
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.javacore/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java
| // Register RootRedirectServlet to handle requests to "/" path | ||
| // This is necessary because without a servlet registered for "/", requests to the root path | ||
| // would result in a 404 error being forwarded to the error page instead of being handled | ||
| // by handleSecurity in CarbonSecuredHttpContext | ||
| Servlet rootRedirectServlet = new RootRedirectServlet(carbonUIDefinitions); | ||
| Dictionary<String, String> rootRedirectServletProps = new Hashtable<>(); | ||
| rootRedirectServletProps.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "/"); | ||
| rootRedirectServletProps.put("osgi.http.whiteboard.context.select", | ||
| "(osgi.http.whiteboard.context.name=carbonContext)"); | ||
| context.registerService(Servlet.class, rootRedirectServlet, rootRedirectServletProps); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "RootRedirectServlet.java"Repository: wso2/carbon-kernel
Length of output: 232
🏁 Script executed:
fd -type f -name "CarbonUIServiceComponent.java"Repository: wso2/carbon-kernel
Length of output: 232
🏁 Script executed:
git ls-files | grep -i "root" | head -20Repository: wso2/carbon-kernel
Length of output: 358
🏁 Script executed:
cat -n "core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java"Repository: wso2/carbon-kernel
Length of output: 5713
🏁 Script executed:
cat -n "core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java" | sed -n '250,275p'Repository: wso2/carbon-kernel
Length of output: 2246
🌐 Web query:
OSGi HTTP Whiteboard servlet pattern "/" exact root match vs default mapping unmatched paths
💡 Result:
In OSGi HTTP Whiteboard, osgi.http.whiteboard.servlet.pattern uses the Servlet URL-pattern rules (Servlet spec §12.2), and the key distinction is:
-
"/"is not an “exact root-only” match. A pattern consisting of only"/"registers the servlet as the default servlet for that ServletContextHelper (context path). It will be selected for any request path that is not matched by a more specific exact/path/extension mapping. (docs.osgi.org)
Example (context path/app): if no other servlet matches, your"/"servlet can receive/app/foo,/app/foo/bar, etc. -
""(empty string) is the exact context root match. The empty-string pattern maps only to the application/context root URL of the formhttp://host:port/<context-root>/. (docs.osgi.org)
Example (context path/app): matches/app/(root), but not/app/foo.
So:
- If you want a catch-all fallback for otherwise-unmatched paths: use
"/". - If you want only the context root (
/.../): use"".
Citations:
- 1: https://docs.osgi.org/specification/osgi.enterprise/7.0.0/service.http.whiteboard.html?utm_source=openai
- 2: https://docs.osgi.org/specification/osgi.enterprise/7.0.0/service.http.whiteboard.html?utm_source=openai
🌐 Web query:
Servlet URL pattern "/" specification exact match or default handler
💡 Result:
In the Servlet spec, a url-pattern of "/" is not an exact match. It designates the application’s default servlet mapping. For this mapping, the container sets:
servletPath = requestURI - contextPathpathInfo = null(jakarta.ee)
If you want an exact match for the context root (i.e., only http://host:port/<context-root>/), the spec defines a special pattern: the empty string "". In that case:
contextPath = ""servletPath = ""pathInfo = "/"(jakarta.ee)
Related: a pattern ending in "/*" is a path mapping (prefix match), while patterns like "/catalog" are exact matches. (jakarta.ee)
Citations:
- 1: https://jakarta.ee/specifications/servlet/5.0/jakarta-servlet-spec-5.0?utm_source=openai
- 2: https://jakarta.ee/specifications/servlet/5.0/jakarta-servlet-spec-5.0?utm_source=openai
- 3: https://jakarta.ee/specifications/servlet/5.0/jakarta-servlet-spec-5.0?utm_source=openai
"/" servlet pattern registers as default handler for all unmatched paths; add root-only validation.
Pattern "/" in OSGi HTTP Whiteboard is a default servlet mapping—it captures any request not matched by more specific patterns, not just the root URL. Combined with the lack of path validation in RootRedirectServlet.handleRedirect(), unmatched paths will be redirected to the console instead of returning 404.
Either change the pattern to "" (exact context root match per Servlet spec) or add a root-only guard in the servlet. The current implementation doesn't match the stated PR objective to handle root requests only.
Recommended fix: add root path validation
--- a/core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java
+++ b/core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java
@@ -81,6 +81,12 @@ public class RootRedirectServlet extends HttpServlet {
private void handleRedirect(HttpServletRequest request, HttpServletResponse response)
throws IOException {
+ // Only redirect if this is the root path, not other unmatched paths
+ String requestPath = request.getRequestURI().substring(request.getContextPath().length());
+ if (!"/".equals(requestPath)) {
+ response.sendError(HttpServletResponse.SC_NOT_FOUND);
+ return;
+ }
String redirectUrl = getRedirectUrl(request);
if (log.isDebugEnabled()) {Alternatively, change the servlet pattern from "/" to "" for exact root-only matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java`
around lines 257 - 266, The RootRedirectServlet is registered with the pattern
"/" which acts as a default handler for unmatched paths; update the registration
in CarbonUIServiceComponent or the servlet to ensure only the exact root is
handled: either change the servlet registration pattern passed to
context.registerService(Servlet.class, rootRedirectServlet,
rootRedirectServletProps) from "/" to an empty pattern "" for exact context-root
matching, or add a root-only guard inside RootRedirectServlet (e.g., in
handleRedirect() / doGet()) that checks the request path
(request.getRequestURI() and/or request.getContextPath()) and only performs the
redirect when the path is exactly the context root ("/" or empty), otherwise
respond with 404/chain to next handler.
| if (carbonUIDefinitions != null && | ||
| carbonUIDefinitions.getContexts().containsKey("default-context")) { | ||
| defaultContext = carbonUIDefinitions.getContexts().get("default-context"); |
There was a problem hiding this comment.
Guard getContexts() before dereference to avoid 500s on root requests.
Line 103 dereferences carbonUIDefinitions.getContexts() without a null check. If contexts are not initialized, this throws and breaks the root redirect flow.
💡 Proposed fix
- if (carbonUIDefinitions != null &&
- carbonUIDefinitions.getContexts().containsKey("default-context")) {
- defaultContext = carbonUIDefinitions.getContexts().get("default-context");
+ if (carbonUIDefinitions != null
+ && carbonUIDefinitions.getContexts() != null
+ && carbonUIDefinitions.getContexts().containsKey("default-context")) {
+ defaultContext = carbonUIDefinitions.getContexts().get("default-context");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (carbonUIDefinitions != null && | |
| carbonUIDefinitions.getContexts().containsKey("default-context")) { | |
| defaultContext = carbonUIDefinitions.getContexts().get("default-context"); | |
| if (carbonUIDefinitions != null | |
| && carbonUIDefinitions.getContexts() != null | |
| && carbonUIDefinitions.getContexts().containsKey("default-context")) { | |
| defaultContext = carbonUIDefinitions.getContexts().get("default-context"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java`
around lines 102 - 104, The code in RootRedirectServlet dereferences
carbonUIDefinitions.getContexts() without checking for null which can cause a
500; update the conditional around carbonUIDefinitions to also check that
carbonUIDefinitions.getContexts() != null before calling containsKey/get, e.g.
guard the expression using carbonUIDefinitions != null &&
carbonUIDefinitions.getContexts() != null &&
carbonUIDefinitions.getContexts().containsKey("default-context") and then set
defaultContext from carbonUIDefinitions.getContexts().get("default-context").
| // Register RootRedirectServlet to handle requests to "/" path | ||
| // This is necessary because without a servlet registered for "/", requests to the root path | ||
| // would result in a 404 error being forwarded to the error page instead of being handled | ||
| // by handleSecurity in CarbonSecuredHttpContext |
There was a problem hiding this comment.
Shall we use javadoc multi line commenting style.
| public RootRedirectServlet() { | ||
| this.carbonUIDefinitions = null; |
There was a problem hiding this comment.
| public RootRedirectServlet() { | |
| this.carbonUIDefinitions = null; | |
| public RootRedirectServlet() { | |
| this.carbonUIDefinitions = null; |
madurangasiriwardena
force-pushed
the
root-path
branch
from
0c62083 to
11d4bf9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java (1)
107-110:⚠️ Potential issue | 🟠 MajorGuard
getContexts()before use to avoid NPE on root hits.At Line 108,
carbonUIDefinitions.getContexts()is used without a null check. If contexts are not initialized, this can throw and break root redirect behavior.Proposed fix
- if (carbonUIDefinitions != null && - carbonUIDefinitions.getContexts().containsKey("default-context")) { + if (carbonUIDefinitions != null && + carbonUIDefinitions.getContexts() != null && + carbonUIDefinitions.getContexts().containsKey("default-context")) { defaultContext = carbonUIDefinitions.getContexts().get("default-context"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java` around lines 107 - 110, The code in RootRedirectServlet uses carbonUIDefinitions.getContexts() without guarding for null, which can cause an NPE on root hits; update the block that sets defaultContext (referencing carbonUIDefinitions, getContexts(), defaultContext) to first retrieve the contexts into a local variable and check that it is not null (and that it containsKey("default-context")) before calling get("default-context"), so you only call containsKey/get when getContexts() returns a non-null Map.core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java (1)
257-266:⚠️ Potential issue | 🟠 MajorUse exact-root servlet mapping instead of default mapping.
Line 263 registers
RootRedirectServletwith pattern"/", which acts as the default servlet mapping and can capture non-root unmatched paths. That can redirect unexpected 404s to the console instead of handling only the server root.Proposed fix
- rootRedirectServletProps.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "/"); + rootRedirectServletProps.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "");OSGi HTTP Whiteboard: does servlet pattern "/" act as default mapping while "" matches only exact context root?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java` around lines 257 - 266, The servlet is registered with the default mapping "/" which can match many paths; change the HTTP_WHITEBOARD_SERVLET_PATTERN value in rootRedirectServletProps to the exact-root mapping (use "" as the pattern) so RootRedirectServlet only handles the context root; update the map entry where HTTP_WHITEBOARD_SERVLET_PATTERN is set (currently in the registration for RootRedirectServlet) to use an empty string instead of "/".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java`:
- Around line 257-266: The servlet is registered with the default mapping "/"
which can match many paths; change the HTTP_WHITEBOARD_SERVLET_PATTERN value in
rootRedirectServletProps to the exact-root mapping (use "" as the pattern) so
RootRedirectServlet only handles the context root; update the map entry where
HTTP_WHITEBOARD_SERVLET_PATTERN is set (currently in the registration for
RootRedirectServlet) to use an empty string instead of "/".
In
`@core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.java`:
- Around line 107-110: The code in RootRedirectServlet uses
carbonUIDefinitions.getContexts() without guarding for null, which can cause an
NPE on root hits; update the block that sets defaultContext (referencing
carbonUIDefinitions, getContexts(), defaultContext) to first retrieve the
contexts into a local variable and check that it is not null (and that it
containsKey("default-context")) before calling get("default-context"), so you
only call containsKey/get when getContexts() returns a non-null Map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 84ed1cc5-071e-47b1-a2b7-75730ec32436
📒 Files selected for processing (2)
core/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/RootRedirectServlet.javacore/org.wso2.carbon.ui/src/main/java/org/wso2/carbon/ui/internal/CarbonUIServiceComponent.java
4 participants
Resolves wso2/product-is#26781