Update RequestNormalizationValve to support post requests#4555
Update RequestNormalizationValve to support post requests#4555hwupathum merged 1 commit intowso2:4.12.xfrom
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...on.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.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 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Actionable comments posted: 1
🤖 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`:
- Around line 40-43: RequestNormalizationValve is currently normalizing the
decoded URI and writing that back to requestURI(), which strips
percent-encoding; instead read the raw encoded form via request.getRequestURI(),
trim trailing slashes from that raw string and write the cleaned raw value to
request.getCoyoteRequest().requestURI().setString(...), then determine how many
trailing slashes were removed and remove the same count from
request.getDecodedRequestURI()/request.getCoyoteRequest().decodedURI() before
calling setString(...) so the decoded and encoded URIs remain consistent and
percent-encoding is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d7f1f490-26ab-4aa7-9760-0bfef7843955
📒 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
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/RequestNormalizationValve.java (1)
41-43:⚠️ Potential issue | 🟠 MajorUse raw-URI slash detection and synchronized trimming to avoid URI state divergence.
At Line 41, normalization is gated by
decodedUri, but Line 42 and Line 43 trim raw/decoded values independently. If trailing/exists only after decoding (e.g., encoded solidus behavior),requestURIanddecodedURIcan become inconsistent.🔧 Suggested patch
- if (decodedUri != null && decodedUri.length() > 1 && decodedUri.endsWith("/")) { - request.getCoyoteRequest().requestURI().setString(rawUri.replaceAll("/+$", "")); - request.getCoyoteRequest().decodedURI().setString(decodedUri.replaceAll("/+$", "")); - } + if (rawUri != null && rawUri.length() > 1 && rawUri.endsWith("/")) { + String normalizedRawUri = rawUri.replaceAll("/+$", ""); + request.getCoyoteRequest().requestURI().setString(normalizedRawUri); + + if (decodedUri != null) { + int removedFromRaw = rawUri.length() - normalizedRawUri.length(); + int maxRemovableFromDecoded = Math.max(0, decodedUri.length() - 1); // keep at least "/" + int toRemoveFromDecoded = Math.min(removedFromRaw, maxRemovableFromDecoded); + String normalizedDecodedUri = + decodedUri.substring(0, decodedUri.length() - toRemoveFromDecoded); + request.getCoyoteRequest().decodedURI().setString(normalizedDecodedUri); + } + }Verification (read-only) to confirm whether this edge case is reachable in this deployment:
#!/bin/bash set -euo pipefail # 1) Check connector options that control encoded slash handling. rg -n --type=xml -C2 'encodedSolidusHandling|allowEncodedSlash' # 2) Re-open the valve implementation under review. fd RequestNormalizationValve.java --exec sed -n '30,90p' {} # 3) Locate other URI raw/decoded handling assumptions in Java code. rg -n --type=java -C2 'getRequestURI\(\)|getDecodedRequestURI\(\)|requestURI\(\)\.setString|decodedURI\(\)\.setString'🤖 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/RequestNormalizationValve.java` around lines 41 - 43, The valve currently gates normalization on decodedUri but trims rawUri and decodedUri independently, which can produce divergent requestURI/decodedURI when trailing slashes exist only after decoding; in RequestNormalizationValve update the logic to inspect the rawUri (from request.getCoyoteRequest().requestURI().toString() or equivalent) for trailing slashes, compute a single trimmedUri once (e.g., remove trailing "/+" from the raw representation or its decoded counterpart as appropriate) and then set both request.getCoyoteRequest().requestURI().setString(...) and request.getCoyoteRequest().decodedURI().setString(...) to that same trimmed value inside the same conditional so both representations remain synchronized (ensure the check uses raw-URI slash detection and apply trimming in one place).
🤖 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/RequestNormalizationValve.java`:
- Around line 41-43: The valve currently gates normalization on decodedUri but
trims rawUri and decodedUri independently, which can produce divergent
requestURI/decodedURI when trailing slashes exist only after decoding; in
RequestNormalizationValve update the logic to inspect the rawUri (from
request.getCoyoteRequest().requestURI().toString() or equivalent) for trailing
slashes, compute a single trimmedUri once (e.g., remove trailing "/+" from the
raw representation or its decoded counterpart as appropriate) and then set both
request.getCoyoteRequest().requestURI().setString(...) and
request.getCoyoteRequest().decodedURI().setString(...) to that same trimmed
value inside the same conditional so both representations remain synchronized
(ensure the check uses raw-URI slash detection and apply trimming in one place).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a9910efc-b05a-4b78-95b5-646de9a3ba05
📒 Files selected for processing (1)
core/org.wso2.carbon.tomcat.ext/src/main/java/org/wso2/carbon/tomcat/ext/valves/RequestNormalizationValve.java
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
This pull request updates the
RequestNormalizationValveto improve how trailing slashes are handled in incoming request URIs. Instead of redirecting clients (which could cause unnecessary round-trips and issues with HTTP methods other than GET), the valve now strips trailing slashes directly on the internal request object before routing. This change ensures all HTTP methods are supported and preserves the request body.Request URI normalization improvements:
Related PR #4552