This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13362 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 6b721f8aa2ae0090cd915ca4e3f4d0e5d7f3de93 Author: Joerg Hoh <[email protected]> AuthorDate: Wed Sep 23 20:29:46 2026 +0200 SLING-13362 fix parameter parsing --- .../engine/impl/parameters/ParameterSupport.java | 24 ++-- .../apache/sling/engine/impl/parameters/Util.java | 52 ++++++-- .../impl/parameters/ParameterSupportTest.java | 140 +++++++++++++++++++++ .../sling/engine/impl/parameters/UtilTest.java | 45 +++++++ 4 files changed, 240 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index 967b9ff..98bed3c 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -265,7 +265,11 @@ public class ParameterSupport { if (query != null) { try { InputStream input = Util.toInputStream(query); - Util.parseQueryString(input, encoding, parameters, false); + // the query string is always decoded with the byte-preserving + // ISO-8859-1 encoding and later fixed up via the '_charset_' + // parameter or the configured default encoding (see + // Util.fixEncoding). + Util.parseQueryString(input, Util.ENCODING_DIRECT, parameters, false); addContainerParameters = checkForAdditionalParameters; } catch (IllegalArgumentException e) { this.log.error("getRequestParameterMapInternal: Error parsing request", e); @@ -379,15 +383,17 @@ public class ParameterSupport { return false; } - // This check assumes the content type ends after the WWW_FORM_URL_ENC - // or continues with blank or semicolon. It will probably break if - // the content type is some string extension of WWW_FORM_URL_ENC - // such as "application/x-www-form-urlencoded-bla" - if (contentType.toLowerCase(Locale.ENGLISH).startsWith(WWW_FORM_URL_ENC)) { - return true; + // only the media type is relevant, optional parameters (such as + // charset) may follow separated by a semicolon. An exact match is + // required: a prefix match would treat unrelated media types such as + // "application/x-www-form-urlencoded-bla" as form encoded content and + // parse a request body which container level inspection does not + // consider to carry parameters + final int semi = contentType.indexOf(';'); + if (semi >= 0) { + contentType = contentType.substring(0, semi); } - - return false; + return WWW_FORM_URL_ENC.equals(contentType.trim().toLowerCase(Locale.ENGLISH)); } private RequestContext getMultiPartContext() { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/Util.java b/src/main/java/org/apache/sling/engine/impl/parameters/Util.java index 4734ab8..03d0852 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/Util.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/Util.java @@ -281,12 +281,7 @@ public class Util { case ESC_NAME: chCode[subState++] = ch; if (subState == chCode.length) { - String code = new String(chCode); - try { - keyBuffer.write(Integer.parseInt(code, 16)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Bad escape sequence: %" + code); - } + keyBuffer.write(decodePercentEscape(chCode)); state = INSIDE_NAME; } break; @@ -328,12 +323,7 @@ public class Util { case ESC_VALUE: chCode[subState++] = ch; if (subState == chCode.length) { - String code = new String(chCode); - try { - valueBuffer.write(Integer.parseInt(code, 16)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Bad escape sequence: %" + code); - } + valueBuffer.write(decodePercentEscape(chCode)); state = INSIDE_VALUE; } break; @@ -345,11 +335,49 @@ public class Util { } } + // a truncated escape sequence at the end of the input is malformed; + // reject it instead of silently dropping it, so that the resulting + // parameter values never differ from a standards compliant decoder + if (state == ESC_NAME || state == ESC_VALUE) { + throw new IllegalArgumentException("Bad escape sequence: unexpected end of input after '%'"); + } + if (keyBuffer.size() > 0) { addNVPair(map, keyBuffer, valueBuffer, encoding, prependNew); } } + /** + * Decodes a two character percent escape sequence, accepting only ASCII + * hexadecimal digits as mandated by RFC 3986. + * + * @param chCode the two escape characters following '%'. Callers must + * guarantee that this array has a length of exactly 2 + * @return the decoded byte value (0..255) + * @throws IllegalArgumentException if a character is not a hex digit + */ + private static int decodePercentEscape(final char[] chCode) { + final int hi = hexDigit(chCode[0]); + final int lo = hexDigit(chCode[1]); + if (hi < 0 || lo < 0) { + throw new IllegalArgumentException("Bad escape sequence: %" + new String(chCode)); + } + return (hi << 4) + lo; + } + + private static int hexDigit(final char c) { + if (c >= '0' && c <= '9') { + return c - '0'; + } + if (c >= 'a' && c <= 'f') { + return c - 'a' + 10; + } + if (c >= 'A' && c <= 'F') { + return c - 'A' + 10; + } + return -1; + } + private static void addNVPair( ParameterMap map, ByteArrayOutputStream keyBuffer, diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java new file mode 100644 index 0000000..b67b4df --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.engine.impl.parameters; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.util.Collections; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Regression tests for SLING-13362 (f010): the engine's request parameter + * decoding must not diverge from a standards compliant (container/WAF) + * decoder, otherwise a client can smuggle data past inspection that the + * engine still acts on. + */ +public class ParameterSupportTest { + + @Test + public void testQueryStringDecodingIgnoresClientControlledCharacterEncoding() throws Exception { + // the client claims (via Content-Type) that the request uses UTF-16BE. + // Per HTTP this only applies to the request body, not the query + // string. If the engine honored it for the query string anyway, the + // two raw bytes 0x00 0x41 would be read as a single UTF-16BE + // character ('A'), rather than as two ISO-8859-1 bytes/characters - + // exactly the kind of divergence a container or WAF (which decodes + // the query string as its own configured/default charset) would not + // see. + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("GET"); + when(request.getCharacterEncoding()).thenReturn("UTF-16BE"); + when(request.getQueryString()).thenReturn("a=%00A"); + when(request.getAttribute(ParameterSupport.MARKER_IS_SERVICE_PROCESSING)) + .thenReturn(null); + when(request.getParameterMap()).thenReturn(Collections.emptyMap()); + + final ParameterSupport parameterSupport = ParameterSupport.getInstance(request); + + assertEquals( + "query string must always be decoded byte-for-byte as ISO-8859-1, " + + "never with the client supplied (body) character encoding", + "\u0000A", + parameterSupport.getParameter("a")); + } + + @Test + public void testWwwFormEncodedContentTypeExactMatchIsParsedAsFormParameters() throws Exception { + final HttpServletRequest request = postRequest("application/x-www-form-urlencoded", "a=b"); + + final ParameterSupport parameterSupport = ParameterSupport.getInstance(request); + + assertEquals("b", parameterSupport.getParameter("a")); + } + + @Test + public void testWwwFormEncodedContentTypeWithCharsetParameterIsParsedAsFormParameters() throws Exception { + final HttpServletRequest request = postRequest("application/x-www-form-urlencoded; charset=UTF-8", "a=b"); + + final ParameterSupport parameterSupport = ParameterSupport.getInstance(request); + + assertEquals("b", parameterSupport.getParameter("a")); + } + + @Test + public void testContentTypeThatOnlyStartsWithFormEncodedMediaTypeIsNotParsedAsFormParameters() throws Exception { + // a container/perimeter doing an exact media type match considers + // this request to carry no parameters at all; if the engine parsed + // the body anyway (prefix match) it would act on data invisible to + // that inspection. + final HttpServletRequest request = postRequest("application/x-www-form-urlencoded-bla", "a=b"); + + final ParameterSupport parameterSupport = ParameterSupport.getInstance(request); + + assertNull(parameterSupport.getParameter("a")); + } + + private static HttpServletRequest postRequest(final String contentType, final String body) throws IOException { + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getMethod()).thenReturn("POST"); + when(request.getCharacterEncoding()).thenReturn("UTF-8"); + when(request.getContentType()).thenReturn(contentType); + when(request.getContentLength()).thenReturn(body.length()); + when(request.getQueryString()).thenReturn(null); + when(request.getAttribute(ParameterSupport.MARKER_IS_SERVICE_PROCESSING)) + .thenReturn(null); + when(request.getParameterMap()).thenReturn(Collections.emptyMap()); + when(request.getInputStream()).thenReturn(toServletInputStream(body)); + return request; + } + + private static ServletInputStream toServletInputStream(final String content) throws UnsupportedEncodingException { + final ByteArrayInputStream in = new ByteArrayInputStream(content.getBytes(Util.ENCODING_DIRECT)); + return new ServletInputStream() { + @Override + public boolean isFinished() { + return in.available() == 0; + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setReadListener(ReadListener readListener) { + // not needed for these tests + } + + @Override + public int read() { + return in.read(); + } + }; + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/UtilTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/UtilTest.java index d10cda2..7bde849 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/UtilTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/UtilTest.java @@ -127,4 +127,49 @@ public class UtilTest extends TestCase { assertEquals("Some Page", map.getStringValue("title")); assertEquals("/content/geometrixx", map.getStringValue("parentPath")); } + + public void test_decode_valid_escapes() throws Exception { + final ParameterMap map = new ParameterMap(); + final String query = "a=%41%62c&b=x%2fy&c=%0d%0A"; + Util.parseQueryString( + new ByteArrayInputStream(query.getBytes(Util.ENCODING_DIRECT)), Util.ENCODING_DIRECT, map, false); + assertEquals("Abc", map.getStringValue("a")); + assertEquals("x/y", map.getStringValue("b")); + assertEquals("\r\n", map.getStringValue("c")); + } + + public void test_signed_escape_sequences_rejected() throws Exception { + // Integer.parseInt(s, 16) accepts a leading sign, a standards compliant + // URL decoder does not: %+d/%+a would decode to CR/LF invisible to any + // RFC 3986 decoder (e.g. a WAF), %-1 to byte 0xFF + for (String query : new String[] {"r=x%+d%+ay", "r=x%-1", "%+d=x", "r=%4+"}) { + try { + Util.parseQueryString( + new ByteArrayInputStream(query.getBytes(Util.ENCODING_DIRECT)), + Util.ENCODING_DIRECT, + new ParameterMap(), + false); + fail("Expected IllegalArgumentException for query: " + query); + } catch (IllegalArgumentException expected) { + // expected: signed/non-hex escape sequences are malformed + } + } + } + + public void test_trailing_incomplete_escape_rejected() throws Exception { + // a trailing "%2" was silently swallowed before, yielding value "b" + // for "a=b%2" - malformed input must be rejected, not truncated + for (String query : new String[] {"a=b%2", "a=b%", "a%2"}) { + try { + Util.parseQueryString( + new ByteArrayInputStream(query.getBytes(Util.ENCODING_DIRECT)), + Util.ENCODING_DIRECT, + new ParameterMap(), + false); + fail("Expected IllegalArgumentException for query: " + query); + } catch (IllegalArgumentException expected) { + // expected: incomplete trailing escape sequence + } + } + } }
