Implement redirection for trailing slash#4552
Merged
hwupathum merged 3 commits intowso2:4.12.xfrom Mar 24, 2026
Merged
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
....carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java
Outdated
Show resolved
Hide resolved
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 |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdded a new Tomcat valve that inspects the decoded request URI and, when it ends with trailing slashes (and has no query string), removes those slashes and issues a redirect; otherwise it delegates to the next valve. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java (1)
47-56:⚠️ Potential issue | 🔴 CriticalMissing
returnaftersendRedirect()causes request to continue processing.After calling
response.sendRedirect(uri), the code falls through and invokesgetNext().invoke(request, response)on line 55. This is incorrect because the response is already committed after the redirect, and continuing to process the request can causeIllegalStateExceptionor undefined behavior.Additionally, the query string is lost since
getDecodedRequestURI()returns only the path. Requests like/path/?foo=barwill redirect to/pathinstead of/path?foo=bar.🐛 Proposed fix
// Handle trailing slash redirect String uri = request.getDecodedRequestURI(); if (uri != null && uri.length() > 1 && uri.endsWith("/")) { uri = uri.replaceAll("/+$", ""); + String queryString = request.getQueryString(); + if (queryString != null && !queryString.isEmpty()) { + uri = uri + "?" + queryString; + } response.sendRedirect(uri); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java` around lines 47 - 56, After sending the redirect in RequestEncodingValve, stop further processing and preserve the original query string: when you call response.sendRedirect(...), build the redirect target from request.getDecodedRequestURI() plus (if non-empty) "?" + request.getQueryString(), call response.sendRedirect(target) and immediately return so getNext().invoke(request, response) is not executed; update the block around getDecodedRequestURI(), response.sendRedirect(...) and getNext().invoke(...) accordingly.
🧹 Nitpick comments (1)
core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java (1)
51-51: Consider using 301 (permanent) redirect for URL normalization.
sendRedirect()defaults to HTTP 302 (temporary redirect). For canonical URL normalization like trailing slash removal, a 301 (permanent redirect) is typically more appropriate as it signals to search engines and browsers that this is the canonical URL, improving SEO and enabling client-side caching.♻️ Proposed refactor using 301 redirect
- response.sendRedirect(uri); + response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); + response.setHeader("Location", uri);This requires adding the import:
import javax.servlet.http.HttpServletResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java` at line 51, In RequestEncodingValve (the code that currently calls response.sendRedirect(uri)), change the redirect to a permanent 301 by setting the response status to HttpServletResponse.SC_MOVED_PERMANENTLY and setting the "Location" header to the target URI instead of calling sendRedirect; also add the import javax.servlet.http.HttpServletResponse so the constant is available and ensure the method returns/ends the request after setting the header/status.
🤖 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.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java`:
- Around line 47-56: After sending the redirect in RequestEncodingValve, stop
further processing and preserve the original query string: when you call
response.sendRedirect(...), build the redirect target from
request.getDecodedRequestURI() plus (if non-empty) "?" +
request.getQueryString(), call response.sendRedirect(target) and immediately
return so getNext().invoke(request, response) is not executed; update the block
around getDecodedRequestURI(), response.sendRedirect(...) and
getNext().invoke(...) accordingly.
---
Nitpick comments:
In
`@core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java`:
- Line 51: In RequestEncodingValve (the code that currently calls
response.sendRedirect(uri)), change the redirect to a permanent 301 by setting
the response status to HttpServletResponse.SC_MOVED_PERMANENTLY and setting the
"Location" header to the target URI instead of calling sendRedirect; also add
the import javax.servlet.http.HttpServletResponse so the constant is available
and ensure the method returns/ends the request after setting the header/status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a753a283-c0d4-47d0-a20f-993163e61833
📒 Files selected for processing (1)
core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestEncodingValve.java
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.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.java`:
- Line 48: The invoke chain call in RequestNormalizationValve currently calls
getNext().invoke(request, response) without a null check; if this valve is last
it can NPE. Update the RequestNormalizationValve.invoke method to guard the call
by obtaining the next valve via getNext(), check it for null, and only call
next.invoke(request, response) when non-null (otherwise simply return or proceed
accordingly), mirroring defensive checks used by other valves in this module.
- Around line 37-46: The current RequestNormalizationValve code (in
RequestNormalizationValve class, inside its request handling method) only
rewrites request.getCoyoteRequest().requestURI()/decodedURI() in-place for paths
ending with "/" but does not redirect the client; change this to send an
external redirect and stop processing: compute cleanDecoded (decodedUri without
trailing slashes) and the corresponding encoded cleanRaw, build the full
redirect target including any query string, set response status to a canonical
redirect code (e.g., 301) and Location header (or call response.sendRedirect) to
that target, and then return immediately from the valve method so no further
processing occurs instead of mutating the internal request objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0df91e79-8be6-407d-bda6-f98701cfc49c
📒 Files selected for processing (1)
core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.java
...on.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.java
Outdated
Show resolved
Hide resolved
...on.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.java
Outdated
Show resolved
Hide resolved
piraveena
previously approved these changes
Mar 24, 2026
piraveena
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new valve to the Tomcat extension module to help normalize incoming request URIs. The main addition is a
RequestNormalizationValveclass that automatically redirects requests with trailing slashes (except for the root path and requests with query strings) to their normalized form without trailing slashes.New request normalization feature:
RequestNormalizationValveclass inorg.wso2.carbon.tomcat.ext.valves, which intercepts requests and redirects URIs ending with trailing slashes (except for root and requests with query strings) to their normalized form without trailing slashes.Related issue wso2/product-is#27030