Skip to content

Commit 130d36d

Browse files
markt-asfmarkt-asf
committed
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
1 parent b14ad16 commit 130d36d

File tree

4 files changed

+131
-17
lines changed

4 files changed

+131
-17
lines changed

‎java/org/apache/catalina/valves/rewrite/RewriteValve.java‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ public void invoke(Request request, Response response) throws IOException, Servl
326326

327327
// As long as MB isn't a char sequence or affiliated, this has to be converted to a string
328328
Charset uriCharset = request.getConnector().getURICharset();
329-
String originalQueryStringEncoded = request.getQueryString();
329+
String queryStringOriginalEncoded = request.getQueryString();
330330
MessageBytes urlMB = context ? request.getRequestPathMB() : request.getDecodedRequestURIMB();
331331
urlMB.toChars();
332332
CharSequence urlDecoded = urlMB.getCharChunk();
@@ -427,18 +427,18 @@ public void invoke(Request request, Response response) throws IOException, Servl
427427
StringBuilder urlStringEncoded =
428428
new StringBuilder(REWRITE_DEFAULT_ENCODER.encode(urlStringRewriteEncoded, uriCharset));
429429

430-
if (!qsd && originalQueryStringEncoded != null && !originalQueryStringEncoded.isEmpty()) {
430+
if (!qsd && queryStringOriginalEncoded != null && !queryStringOriginalEncoded.isEmpty()) {
431431
if (rewrittenQueryStringRewriteEncoded == null) {
432432
urlStringEncoded.append('?');
433-
urlStringEncoded.append(originalQueryStringEncoded);
433+
urlStringEncoded.append(queryStringOriginalEncoded);
434434
} else {
435435
if (qsa) {
436436
// if qsa is specified append the query
437437
urlStringEncoded.append('?');
438438
urlStringEncoded.append(
439439
REWRITE_QUERY_ENCODER.encode(rewrittenQueryStringRewriteEncoded, uriCharset));
440440
urlStringEncoded.append('&');
441-
urlStringEncoded.append(originalQueryStringEncoded);
441+
urlStringEncoded.append(queryStringOriginalEncoded);
442442
} else if (index == urlStringEncoded.length() - 1) {
443443
// if the ? is the last character delete it, its only purpose was to
444444
// prevent the rewrite module from appending the query string
@@ -554,24 +554,31 @@ public void invoke(Request request, Response response) throws IOException, Servl
554554

555555
// Step 3. Complete the 2nd stage to encoding.
556556
chunk.append(REWRITE_DEFAULT_ENCODER.encode(urlStringRewriteEncoded, uriCharset));
557-
// Decoded and normalized URI
558-
// Rewriting may have denormalized the URL
559-
urlStringRewriteEncoded = RequestUtil.normalize(urlStringRewriteEncoded);
557+
// Rewriting may have denormalized the URL and added encoded characters
558+
// Decode then normalize
559+
String urlStringRewriteDecoded = URLDecoder.decode(urlStringRewriteEncoded, uriCharset);
560+
urlStringRewriteDecoded = RequestUtil.normalize(urlStringRewriteDecoded);
560561
request.getCoyoteRequest().decodedURI().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
561562
chunk = request.getCoyoteRequest().decodedURI().getCharChunk();
562563
if (context) {
563564
// This is decoded and normalized
564565
chunk.append(request.getServletContext().getContextPath());
565566
}
566-
chunk.append(URLDecoder.decode(urlStringRewriteEncoded, uriCharset));
567-
// Set the new Query if there is one
568-
if (queryStringRewriteEncoded != null) {
567+
chunk.append(urlStringRewriteDecoded);
568+
// Set the new Query String
569+
if (queryStringRewriteEncoded == null) {
570+
// No new query string. Therefore the original is retained unless QSD is defined.
571+
if (qsd) {
572+
request.getCoyoteRequest().queryString().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
573+
}
574+
} else {
575+
// New query string. Therefore the original is dropped unless QSA is defined (and QSD is not).
569576
request.getCoyoteRequest().queryString().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
570577
chunk = request.getCoyoteRequest().queryString().getCharChunk();
571578
chunk.append(REWRITE_QUERY_ENCODER.encode(queryStringRewriteEncoded, uriCharset));
572-
if (qsa && originalQueryStringEncoded != null && !originalQueryStringEncoded.isEmpty()) {
579+
if (qsa && queryStringOriginalEncoded != null && !queryStringOriginalEncoded.isEmpty()) {
573580
chunk.append('&');
574-
chunk.append(originalQueryStringEncoded);
581+
chunk.append(queryStringOriginalEncoded);
575582
}
576583
}
577584
// Set the new host if it changed
@@ -666,6 +673,10 @@ public static Object parse(String line) {
666673
while (flagsTokenizer.hasMoreElements()) {
667674
parseRuleFlag(line, rule, flagsTokenizer.nextToken());
668675
}
676+
// If QSD and QSA are present, QSD always takes precedence
677+
if (rule.isQsdiscard()) {
678+
rule.setQsappend(false);
679+
}
669680
}
670681
return rule;
671682
} else if (token.equals("RewriteMap")) {

‎test/org/apache/catalina/startup/TomcatBaseTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ public void service(HttpServletRequest request,
543543
value.append(';');
544544
}
545545
}
546-
out.println("PARAM/" + name + ": " + value);
546+
out.println("PARAM:" + name + ": " + value);
547547
}
548548

549549
out.println("SESSION-REQUESTED-ID: " +

‎test/org/apache/catalina/valves/rewrite/TestRewriteValve.java‎

Lines changed: 103 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -301,17 +301,112 @@ public void testNonAsciiPathRedirect() throws Exception {
301301
}
302302

303303
@Test
304-
public void testQueryString() throws Exception {
304+
public void testQueryStringTargetOnly() throws Exception {
305+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2", "/b/id=1", "/c/id=1", "je=2");
306+
}
307+
308+
@Test
309+
public void testQueryStringTargetOnlyQSA() throws Exception {
310+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSA]", "/b/id=1", "/c/id=1", "je=2");
311+
}
312+
313+
@Test
314+
public void testQueryStringTargetOnlyQSD() throws Exception {
315+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSD]", "/b/id=1", "/c/id=1", "je=2");
316+
}
317+
318+
@Test
319+
public void testQueryStringTargetOnlyQSAQSD() throws Exception {
320+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?je=2 [QSA,QSD]", "/b/id=1", "/c/id=1", "je=2");
321+
}
322+
323+
@Test
324+
public void testQueryStringTargetOnlyQS() throws Exception {
305325
doTestRewrite("RewriteRule ^/b/(.*) /c?$1", "/b/id=1", "/c", "id=1");
306326
}
307327

328+
@Test
329+
public void testQueryStringTargetOnlyQSAQS() throws Exception {
330+
doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSA]", "/b/id=1", "/c", "id=1");
331+
}
332+
333+
@Test
334+
public void testQueryStringTargetOnlyQSDQS() throws Exception {
335+
doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSD]", "/b/id=1", "/c", "id=1");
336+
}
337+
338+
@Test
339+
public void testQueryStringTargetOnlyQSAQSDQS() throws Exception {
340+
doTestRewrite("RewriteRule ^/b/(.*) /c?$1 [QSA,QSD]", "/b/id=1", "/c", "id=1");
341+
}
342+
343+
@Test
344+
public void testQueryStringSourceOnly() throws Exception {
345+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1", "/b/d?id=1", "/c/d", "id=1");
346+
}
347+
348+
@Test
349+
public void testQueryStringSourceOnlyQSA() throws Exception {
350+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSA]", "/b/d?id=1", "/c/d", "id=1");
351+
}
352+
353+
@Test
354+
public void testQueryStringSourceOnlyQSD() throws Exception {
355+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?id=1", "/c/d", null);
356+
}
357+
358+
@Test
359+
public void testQueryStringSourceOnlyQSAQSD() throws Exception {
360+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSA,QSD]", "/b/d?id=1", "/c/d", null);
361+
}
362+
363+
@Test
364+
public void testQueryStringSourceAndTarget() throws Exception {
365+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1", "/b/d?je=2", "/c/d", "id=1");
366+
}
367+
368+
@Test
369+
public void testQueryStringSourceAndTargetQSA() throws Exception {
370+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSA]", "/b/d?je=2", "/c/d", "id=1&je=2");
371+
}
372+
373+
@Test
374+
public void testQueryStringSourceAndTargetQSD() throws Exception {
375+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSD]", "/b/d?je=2", "/c/d", "id=1");
376+
}
377+
378+
@Test
379+
public void testQueryStringSourceAndTargetQSAQSD() throws Exception {
380+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?id=1 [QSA,QSD]", "/b/d?je=2", "/c/d", "id=1");
381+
}
382+
383+
@Test
384+
public void testQueryStringEncoded01() throws Exception {
385+
doTestRewrite("RewriteCond %{QUERY_STRING} a=(.*)\nRewriteRule ^/b.*$ /%1 [QSD]", "/b?a=c", "/c", null);
386+
}
387+
388+
@Test
389+
public void testQueryStringEncoded02() throws Exception {
390+
doTestRewrite("RewriteCond %{QUERY_STRING} a=(.*)\nRewriteRule ^/b.*$ /z/%1 [QSD]", "/b?a=%2e%2e%2fc%2faAbB", "/z/%2e%2e%2fc%2faAbB", null);
391+
}
392+
308393
@Test
309394
public void testQueryStringRemove() throws Exception {
310-
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?=1", "/c/d", null);
395+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?id=1", "/c/d", null);
311396
}
312397

313398
@Test
314399
public void testQueryStringRemove02() throws Exception {
400+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?id=1", "/c/d", null);
401+
}
402+
403+
@Test
404+
public void testQueryStringRemoveInvalid() throws Exception {
405+
doTestRewrite("RewriteRule ^/b/(.*) /c/$1?", "/b/d?=1", "/c/d", null);
406+
}
407+
408+
@Test
409+
public void testQueryStringRemoveInvalid02() throws Exception {
315410
doTestRewrite("RewriteRule ^/b/(.*) /c/$1 [QSD]", "/b/d?=1", "/c/d", null);
316411
}
317412

@@ -616,7 +711,7 @@ public void testUtf8FlagsRBNE() throws Exception {
616711
public void testFlagsNC() throws Exception {
617712
// https://bz.apache.org/bugzilla/show_bug.cgi?id=60116
618713
doTestRewrite("RewriteCond %{QUERY_STRING} a=([a-z]*) [NC]\n" + "RewriteRule .* - [E=X-Test:%1]", "/c?a=aAa",
619-
"/c", null, "aAa");
714+
"/c", "a=aAa", "aAa");
620715
}
621716

622717
@Test
@@ -806,12 +901,16 @@ public void invoke(Request request, Response response) throws IOException, Servl
806901
// were written into the request target
807902
Assert.assertEquals(400, rc);
808903
} else {
904+
// If there is an expected URI, the request should be successful
905+
Assert.assertEquals(200, rc);
809906
String body = res.toString();
810907
RequestDescriptor requestDesc = SnoopResult.parse(body);
811908
String requestURI = requestDesc.getRequestInfo("REQUEST-URI");
812909
Assert.assertEquals(expectedURI, requestURI);
813910

814-
if (expectedQueryString != null) {
911+
if (expectedQueryString == null) {
912+
Assert.assertTrue(requestDesc.getParams().isEmpty());
913+
} else {
815914
String queryString = requestDesc.getRequestInfo("REQUEST-QUERY-STRING");
816915
Assert.assertEquals(expectedQueryString, queryString);
817916
}

‎webapps/docs/changelog.xml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@
117117
when the store was used with the <code>PersistentValve</code>. Based on
118118
pull request <pr>882</pr> by Aaron Ogburn. (markt)
119119
</fix>
120+
<fix>
121+
Fix handling of <code>QSA</code> and <code>QSD</code> flags in
122+
<code>RewriteValve</code>. (markt)
123+
</fix>
120124
</changelog>
121125
</subsection>
122126
<subsection name="Coyote">

0 commit comments

Comments
 (0)