Repository navigation
Fix the servlet encodeURL methods and the root context path - #16564
Conversation
ServletHttpRequestAdapter.getContextPath() and DummyServletContext.getContextPath() returned "/", but the servlet API returns "" for the root context. Code that builds a URL from the context path, such as response.sendRedirect(request.getContextPath() + "/login") or Spring Security's DefaultRedirectStrategy, got "//login" instead of "/login". Fixes apache#16563
ServletHttpResponseAdapter implemented encodeURL, encodeRedirectURL, encodeUrl and encodeRedirectUrl with URLEncoder, so the whole URL was form-encoded and a redirect sent through encodeRedirectURL got a Location such as %2Flogin%3Fnext%3D%2Forders. These methods only add a session id when URL rewriting is used, and otherwise return the URL unchanged. Dubbo does not rewrite URLs for sessions, so return the URL as it is. Fixes apache#16563
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16564 +/- ##
============================================
+ Coverage 59.02% 60.98% +1.95%
Complexity 15 15
============================================
Files 1953 1953
Lines 89284 89284
Branches 13477 13477
============================================
+ Hits 52700 54449 +1749
+ Misses 30877 29242 -1635
+ Partials 5707 5593 -114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
EarthChen
left a comment
There was a problem hiding this comment.
Verified locally on 3.3 (merge-base 1afcdf7883, PR head 984d823420).
Reproduction. With the three main files reverted to the base, the new tests fail: getContextPath() returns / instead of an empty string, encodeRedirectURL("/login?next=/orders") returns %2Flogin%3Fnext%3D%2Forders, and encodeURL encodes the whole URL. On the PR head both test classes pass (1 + 3 tests), and Spotless and checkstyle pass for the module.
Contract. The fix matches the Servlet API: getContextPath() returns an empty string for the root context, and encodeURL / encodeRedirectURL return the URL unchanged when no session id is added. Dubbo does not rewrite URLs for sessions.
Side effects. I found no reader of these getContextPath() implementations. Routing uses the container's getRequestURI(). RequestUtils.encodeURL is still used by the JAX-RS adapter and tests, and the PR removed the now-unused import.
Minor, non-blocking: the empty-string case in ServletHttpResponseAdapterTest also passes on the old code, and no test covers sendRedirect end to end. A non-root context path is still unsupported, as the issue notes.
What is the purpose of the change?
Fixes #16563.
Servlet code running in Triple REST (
dubbo-triple-servlet) sent wrongLocationheaders when it redirected the usual ways:ServletHttpRequestAdapter.getContextPath()andDummyServletContext.getContextPath()returned"/", sosendRedirect(getContextPath() + "/login")sent//login. The servlet API returns""for the root context. They now return"".ServletHttpResponseAdapter.encodeURL,encodeRedirectURL,encodeUrlandencodeRedirectUrlreturnedURLEncoder.encode(url), sosendRedirect(encodeRedirectURL("/login?next=/orders"))sent%2Flogin%3Fnext%3D%2Forders. In the servlet API these methods only add a session id for URL rewriting and otherwise return the URL unchanged. Dubbo does not rewrite URLs for sessions, so they now return the URL as it is.Both are needed for code like Spring Security's
DefaultRedirectStrategy, which callsencodeRedirectURL(getContextPath() + url): with only the second change it would send//login?next=/orders. The PR has one commit for each, the context path first.Behavior change
encodeURL,encodeRedirectURL,encodeUrlandencodeRedirectUrlreturn their argument unchanged instead of a form-encoded copy.HttpServletRequest#getContextPath()andServletContext#getContextPath()return""instead of"/".encodeURLto percent-escape a value now gets the value back unescaped, as with any servlet container.jakartaadapters change the same way, since they are copied from these sources at build time.Not covered
contextpath, orTripleFilterruns in a webapp that is not deployed at the root,getContextPath()now returns""instead of"/"(a redirect togetContextPath() + "/login"goes to/logininstead of//login), but a servlet container would return that context path and leave it out ofgetServletPath(). Doing that needs the context path of the matched service or of the container request, so I left it out of this PR.Verification
ServletHttpRequestAdapterTest(empty context path for the request and the servlet context) andServletHttpResponseAdapterTest(the four methods return/login?next=/orders,http://example.com/a b?x=1#yand""unchanged). They fail before the change (expected: <> but was: </>,expected: </login?next=/orders> but was: <%2Flogin%3Fnext%3D%2Forders>) and pass after it. Putting the old code back into any one of the six methods makes a test fail. The tests use thejavaxclasses because thejakartacopies are only generated on JDK 17+.mvn clean test verifyof dubbo-triple-servlet and dubbo-rest-spring with the options of the CI unit test job, on JDK 8, 11, 17, 21 and 25: 61 tests, 0 failures each time (11 skipped, the existing@DisabledSpringMvcRestProtocolTest); checkstyle and spotless are clean./security):/redirect%2Flogin%3Fnext%3D%2Forders/login?next=/orders/login?next=/orders/context//login?next=/orders/login?next=/orders/login?next=/orders/security%2F%2Flogin%3Fnext%3D%2Forders/login?next=/orders/login?next=/ordersChecklist