This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 1d3404063e Fix a couple of issues with QSA/QSD handling and associated
tests
1d3404063e is described below
commit 1d3404063e48129f3628241a3bffa7160eb771a0
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Aug 29 14:42:14 2025 +0100
Fix a couple of issues with QSA/QSD handling and associated tests
- Original query string was incorrectly retained when there was no new
query string and QSD was set
- QSD did not take precedence when QSA and QSD were both set
Rename variables for consistency / ease of maintenance
---
.../catalina/valves/rewrite/RewriteValve.java | 35 ++++---
.../apache/catalina/startup/TomcatBaseTest.java | 2 +-
.../catalina/valves/rewrite/TestRewriteValve.java | 107 ++++++++++++++++++++-
webapps/docs/changelog.xml | 4 +
4 files changed, 131 insertions(+), 17 deletions(-)
diff --git a/java/org/apache/catalina/valves/rewrite/RewriteValve.java
b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
index 68dfd8feea..9a4b0c3968 100644
--- a/java/org/apache/catalina/valves/rewrite/RewriteValve.java
+++ b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
@@ -325,7 +325,7 @@ public class RewriteValve extends ValveBase {
// As long as MB isn't a char sequence or affiliated, this has to
be converted to a string
Charset uriCharset = request.getConnector().getURICharset();
- String originalQueryStringEncoded = request.getQueryString();
+ String queryStringOriginalEncoded = request.getQueryString();
MessageBytes urlMB = context ? request.getRequestPathMB() :
request.getDecodedRequestURIMB();
urlMB.toChars();
CharSequence urlDecoded = urlMB.getCharChunk();
@@ -426,10 +426,10 @@ public class RewriteValve extends ValveBase {
StringBuilder urlStringEncoded =
new
StringBuilder(REWRITE_DEFAULT_ENCODER.encode(urlStringRewriteEncoded,
uriCharset));
- if (!qsd && originalQueryStringEncoded != null &&
!originalQueryStringEncoded.isEmpty()) {
+ if (!qsd && queryStringOriginalEncoded != null &&
!queryStringOriginalEncoded.isEmpty()) {
if (rewrittenQueryStringRewriteEncoded == null) {
urlStringEncoded.append('?');
-
urlStringEncoded.append(originalQueryStringEncoded);
+
urlStringEncoded.append(queryStringOriginalEncoded);
} else {
if (qsa) {
// if qsa is specified append the query
@@ -437,7 +437,7 @@ public class RewriteValve extends ValveBase {
urlStringEncoded.append(
REWRITE_QUERY_ENCODER.encode(rewrittenQueryStringRewriteEncoded, uriCharset));
urlStringEncoded.append('&');
-
urlStringEncoded.append(originalQueryStringEncoded);
+
urlStringEncoded.append(queryStringOriginalEncoded);
} else if (index == urlStringEncoded.length() - 1)
{
// if the ? is the last character delete it,
its only purpose was to
// prevent the rewrite module from appending
the query string
@@ -553,24 +553,31 @@ public class RewriteValve extends ValveBase {
// Step 3. Complete the 2nd stage to encoding.
chunk.append(REWRITE_DEFAULT_ENCODER.encode(urlStringRewriteEncoded,
uriCharset));
- // Decoded and normalized URI
- // Rewriting may have denormalized the URL
- urlStringRewriteEncoded =
RequestUtil.normalize(urlStringRewriteEncoded);
+ // Rewriting may have denormalized the URL and added
encoded characters
+ // Decode then normalize
+ String urlStringRewriteDecoded =
URLDecoder.decode(urlStringRewriteEncoded, uriCharset);
+ urlStringRewriteDecoded =
RequestUtil.normalize(urlStringRewriteDecoded);
request.getCoyoteRequest().decodedURI().setChars(MessageBytes.EMPTY_CHAR_ARRAY,
0, 0);
chunk =
request.getCoyoteRequest().decodedURI().getCharChunk();
if (context) {
// This is decoded and normalized
chunk.append(request.getServletContext().getContextPath());
}
- chunk.append(URLDecoder.decode(urlStringRewriteEncoded,
uriCharset));
- // Set the new Query if there is one
- if (queryStringRewriteEncoded != null) {
+ chunk.append(urlStringRewriteDecoded);
+ // Set the new Query String
+ if (queryStringRewriteEncoded == null) {
+ // No new query string. Therefore the original is
retained unless QSD is defined.
+ if (qsd) {
+
request.getCoyoteRequest().queryString().setChars(MessageBytes.EMPTY_CHAR_ARRAY,
0, 0);
+ }
+ } else {
+ // New query string. Therefore the original is dropped
unless QSA is defined (and QSD is not).
request.getCoyoteRequest().queryString().setChars(MessageBytes.EMPTY_CHAR_ARRAY,
0, 0);
chunk =
request.getCoyoteRequest().queryString().getCharChunk();
chunk.append(REWRITE_QUERY_ENCODER.encode(queryStringRewriteEncoded,
uriCharset));
- if (qsa && originalQueryStringEncoded != null &&
!originalQueryStringEncoded.isEmpty()) {
+ if (qsa && queryStringOriginalEncoded != null &&
!queryStringOriginalEncoded.isEmpty()) {
chunk.append('&');
- chunk.append(originalQueryStringEncoded);
+ chunk.append(queryStringOriginalEncoded);
}
}
// Set the new host if it changed
@@ -665,6 +672,10 @@ public class RewriteValve extends ValveBase {
while (flagsTokenizer.hasMoreElements()) {
parseRuleFlag(line, rule, flagsTokenizer.nextToken());
}
+ // If QSD and QSA are present, QSD always takes precedence
+ if (rule.isQsdiscard()) {
+ rule.setQsappend(false);
+ }
}
return rule;
} else if (token.equals("RewriteMap")) {
diff --git a/test/org/apache/catalina/startup/TomcatBaseTest.java
b/test/org/apache/catalina/startup/TomcatBaseTest.java
index fabedcfba9..8594e8aa13 100644
--- a/test/org/apache/catalina/startup/TomcatBaseTest.java
+++ b/test/org/apache/catalina/startup/TomcatBaseTest.java
@@ -544,7 +544,7 @@ public abstract class TomcatBaseTest extends
LoggingBaseTest {
value.append(';');
}
}
- out.println("PARAM/" + name + ": " + value);
+ out.println("PARAM:" + name + ": " + value);
}
out.println("SESSION-REQUESTED-ID: " +
diff --git a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
index ce2f7c5d5c..beb6835d29 100644
--- a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
+++ b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
@@ -301,17 +301,112 @@ public class TestRewriteValve extends TomcatBaseTest {
}
@Test
- public void testQueryString() throws Exception {
+ public void testQueryStringTargetOnly() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2", "/b/id=1", "/c/id=1",
"je=2");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQSA() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSA]", "/b/id=1",
"/c/id=1", "je=2");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSD]", "/b/id=1",
"/c/id=1", "je=2");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQSAQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSA,QSD]", "/b/id=1",
"/c/id=1", "je=2");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQS() throws Exception {
doTestRewrite("RewriteRule ^/b/(.*) /c?$1", "/b/id=1", "/c", "id=1");
}
+ @Test
+ public void testQueryStringTargetOnlyQSAQS() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSA]", "/b/id=1", "/c",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQSDQS() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSD]", "/b/id=1", "/c",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringTargetOnlyQSAQSDQS() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSA,QSD]", "/b/id=1", "/c",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringSourceOnly() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1", "/b/d?id=1", "/c/d",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringSourceOnlyQSA() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSA]", "/b/d?id=1", "/c/d",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringSourceOnlyQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?id=1", "/c/d",
null);
+ }
+
+ @Test
+ public void testQueryStringSourceOnlyQSAQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSA,QSD]", "/b/d?id=1",
"/c/d", null);
+ }
+
+ @Test
+ public void testQueryStringSourceAndTarget() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1", "/b/d?je=2", "/c/d",
"id=1");
+ }
+
+ @Test
+ public void testQueryStringSourceAndTargetQSA() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSA]", "/b/d?je=2",
"/c/d", "id=1&je=2");
+ }
+
+ @Test
+ public void testQueryStringSourceAndTargetQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSD]", "/b/d?je=2",
"/c/d", "id=1");
+ }
+
+ @Test
+ public void testQueryStringSourceAndTargetQSAQSD() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSA,QSD]",
"/b/d?je=2", "/c/d", "id=1");
+ }
+
+ @Test
+ public void testQueryStringEncoded01() throws Exception {
+ doTestRewrite("RewriteCond %{QUERY_STRING} a=(.*)\nRewriteRule ^/b.*$
/%1 [QSD]", "/b?a=c", "/c", null);
+ }
+
+ @Test
+ public void testQueryStringEncoded02() throws Exception {
+ doTestRewrite("RewriteCond %{QUERY_STRING} a=(.*)\nRewriteRule ^/b.*$
/z/%1 [QSD]", "/b?a=%2e%2e%2fc%2faAbB", "/z/%2e%2e%2fc%2faAbB", null);
+ }
+
@Test
public void testQueryStringRemove() throws Exception {
- doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?=1", "/c/d", null);
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?id=1", "/c/d",
null);
}
@Test
public void testQueryStringRemove02() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?id=1", "/c/d",
null);
+ }
+
+ @Test
+ public void testQueryStringRemoveInvalid() throws Exception {
+ doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?=1", "/c/d", null);
+ }
+
+ @Test
+ public void testQueryStringRemoveInvalid02() throws Exception {
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?=1", "/c/d",
null);
}
@@ -616,7 +711,7 @@ public class TestRewriteValve extends TomcatBaseTest {
public void testFlagsNC() throws Exception {
// https://bz.apache.org/bugzilla/show_bug.cgi?id=60116
doTestRewrite("RewriteCond %{QUERY_STRING} a=([a-z]*) [NC]\n" +
"RewriteRule .* - [E=X-Test:%1]", "/c?a=aAa",
- "/c", null, "aAa");
+ "/c", "a=aAa", "aAa");
}
@Test
@@ -806,12 +901,16 @@ public class TestRewriteValve extends TomcatBaseTest {
// were written into the request target
Assert.assertEquals(400, rc);
} else {
+ // If there is an expected URI, the request should be successful
+ Assert.assertEquals(200, rc);
String body = res.toString();
RequestDescriptor requestDesc = SnoopResult.parse(body);
String requestURI = requestDesc.getRequestInfo("REQUEST-URI");
Assert.assertEquals(expectedURI, requestURI);
- if (expectedQueryString != null) {
+ if (expectedQueryString == null) {
+ Assert.assertTrue(requestDesc.getParams().isEmpty());
+ } else {
String queryString =
requestDesc.getRequestInfo("REQUEST-QUERY-STRING");
Assert.assertEquals(expectedQueryString, queryString);
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index f0eeb26e40..5145840eb6 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -186,6 +186,10 @@
Refactor <code>WebResource</code> locking to use the new
<code>KeyedReentrantReadWriteLock</code>. (markt)
</scode>
+ <fix>
+ Fix handling of <code>QSA</code> and <code>QSD</code> flags in
+ <code>RewriteValve</code>. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
