Redirect to the prefered console UI when the server root accessed#4499
Redirect to the prefered console UI when the server root accessed#4499madurangasiriwardena wants to merge 1 commit intowso2:4.12.xfrom
Conversation
WalkthroughThe pull request introduces a new RootRedirectServlet that handles requests to the root path ("/") and redirects to a configured default UI context or falls back to a default management UI path. The servlet is registered with the HTTP Whiteboard in CarbonUIServiceComponent to manage root path requests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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
from
908f8c3 to
a750f9b
Compare
madurangasiriwardena
force-pushed
the
root-path
branch
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").
3 participants
Resolves wso2/product-is#26781
Summary by CodeRabbit