From 9c1a1916962354f1e69fe43107219c25a86e6cef Mon Sep 17 00:00:00 2001 From: David Dossot Date: Fri, 24 Feb 2017 12:45:23 -0800 Subject: [PATCH] Trim optional white space in CombinedHttpHeaders values Motivation: The updated HTTP/1.x RFC allows for header values to be CSV and separated by OWS [1]. CombinedHttpHeaders should remove this OWS on insertion. [1] https://tools.ietf.org/html/rfc7230#section-7 Modification: CombinedHttpHeaders doesn't account for the OWS and returns it back to the user as part of the value. Result: Fixes #6452 --- .../codec/http/CombinedHttpHeaders.java | 18 ++- .../netty/handler/codec/http/HttpHeaders.java | 2 +- .../codec/http/CombinedHttpHeadersTest.java | 31 +++++ .../io/netty/util/internal/StringUtil.java | 123 +++++++++++++++--- .../netty/util/internal/StringUtilTest.java | 64 ++++++++- 5 files changed, 210 insertions(+), 28 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java index 231cf1d994..7d9c3b185f 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java @@ -16,9 +16,9 @@ package io.netty.handler.codec.http; import io.netty.handler.codec.DefaultHeaders; +import io.netty.handler.codec.Headers; import io.netty.handler.codec.ValueConverter; import io.netty.util.HashingStrategy; -import io.netty.handler.codec.Headers; import io.netty.util.internal.StringUtil; import java.util.Collection; @@ -39,6 +39,11 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { super(new CombinedHttpHeadersImpl(CASE_INSENSITIVE_HASHER, valueConverter(validate), nameValidator(validate))); } + @Override + public boolean containsValue(CharSequence name, CharSequence value, boolean ignoreCase) { + return super.containsValue(name, StringUtil.trimOws(value), ignoreCase); + } + private static final class CombinedHttpHeadersImpl extends DefaultHeaders { /** @@ -53,7 +58,7 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { objectEscaper = new CsvValueEscaper() { @Override public CharSequence escape(Object value) { - return StringUtil.escapeCsv(valueConverter().convertObject(value)); + return StringUtil.escapeCsv(valueConverter().convertObject(value), true); } }; } @@ -65,7 +70,7 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { charSequenceEscaper = new CsvValueEscaper() { @Override public CharSequence escape(CharSequence value) { - return StringUtil.escapeCsv(value); + return StringUtil.escapeCsv(value, true); } }; } @@ -136,7 +141,7 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { @Override public CombinedHttpHeadersImpl add(CharSequence name, CharSequence value) { - return addEscapedValue(name, StringUtil.escapeCsv(value)); + return addEscapedValue(name, charSequenceEscaper().escape(value)); } @Override @@ -149,6 +154,11 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { return addEscapedValue(name, commaSeparate(charSequenceEscaper(), values)); } + @Override + public CombinedHttpHeadersImpl addObject(CharSequence name, Object value) { + return addEscapedValue(name, commaSeparate(objectEscaper(), value)); + } + @Override public CombinedHttpHeadersImpl addObject(CharSequence name, Iterable values) { return addEscapedValue(name, commaSeparate(objectEscaper(), values)); diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java index 2b5d75faec..ed1d776377 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java @@ -1573,7 +1573,7 @@ public abstract class HttpHeaders implements Iterable> /** * Returns {@code true} if a header with the {@code name} and {@code value} exists, {@code false} otherwise. - * This also handles multiple values that are seperated with a {@code ,}. + * This also handles multiple values that are separated with a {@code ,}. *

* If {@code ignoreCase} is {@code true} then a case insensitive compare is done on the value. * @param name the name of the header to find diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java index 6ec9bfaf36..258208f462 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java @@ -16,6 +16,7 @@ package io.netty.handler.codec.http; import io.netty.handler.codec.http.HttpHeadersTestUtils.HeaderValue; +import io.netty.util.internal.StringUtil; import org.junit.Test; import java.util.Arrays; @@ -23,6 +24,8 @@ import java.util.Collections; import static io.netty.util.AsciiString.contentEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; public class CombinedHttpHeadersTest { @@ -271,4 +274,32 @@ public class CombinedHttpHeadersTest { headers.set(HEADER_NAME, "\"a,b,c\""); assertEquals(Arrays.asList("a,b,c"), headers.getAll(HEADER_NAME)); } + + @Test + public void owsTrimming() { + final CombinedHttpHeaders headers = newCombinedHttpHeaders(); + headers.set(HEADER_NAME, Arrays.asList("\ta", " ", " b ", "\t \t")); + headers.add(HEADER_NAME, " c, d \t"); + + assertEquals(Arrays.asList("a", "", "b", "", "c, d"), headers.getAll(HEADER_NAME)); + assertEquals("a,,b,,\"c, d\"", headers.get(HEADER_NAME)); + + assertTrue(headers.containsValue(HEADER_NAME, "a", true)); + assertTrue(headers.containsValue(HEADER_NAME, " a ", true)); + assertTrue(headers.containsValue(HEADER_NAME, "a", true)); + assertFalse(headers.containsValue(HEADER_NAME, "a,b", true)); + + assertFalse(headers.containsValue(HEADER_NAME, " c, d ", true)); + assertFalse(headers.containsValue(HEADER_NAME, "c, d", true)); + assertTrue(headers.containsValue(HEADER_NAME, " c ", true)); + assertTrue(headers.containsValue(HEADER_NAME, "d", true)); + + assertTrue(headers.containsValue(HEADER_NAME, "\t", true)); + assertTrue(headers.containsValue(HEADER_NAME, "", true)); + + assertFalse(headers.containsValue(HEADER_NAME, "e", true)); + + HttpHeaders copiedHeaders = newCombinedHttpHeaders().add(headers); + assertEquals(Arrays.asList("a", "", "b", "", "c, d"), copiedHeaders.getAll(HEADER_NAME)); + } } diff --git a/common/src/main/java/io/netty/util/internal/StringUtil.java b/common/src/main/java/io/netty/util/internal/StringUtil.java index 2ad7d13950..5332464f21 100644 --- a/common/src/main/java/io/netty/util/internal/StringUtil.java +++ b/common/src/main/java/io/netty/util/internal/StringUtil.java @@ -34,6 +34,7 @@ public final class StringUtil { public static final char LINE_FEED = '\n'; public static final char CARRIAGE_RETURN = '\r'; public static final char TAB = '\t'; + public static final char SPACE = 0x20; private static final String[] BYTE2HEX_PAD = new String[256]; private static final String[] BYTE2HEX_NOPAD = new String[256]; @@ -48,16 +49,16 @@ public final class StringUtil { static { // Generate the lookup table that converts a byte into a 2-digit hexadecimal integer. int i; - for (i = 0; i < 10; i ++) { + for (i = 0; i < 10; i++) { BYTE2HEX_PAD[i] = "0" + i; BYTE2HEX_NOPAD[i] = String.valueOf(i); } - for (; i < 16; i ++) { + for (; i < 16; i++) { char c = (char) ('a' + i - 10); BYTE2HEX_PAD[i] = "0" + c; BYTE2HEX_NOPAD[i] = String.valueOf(c); } - for (; i < BYTE2HEX_PAD.length; i ++) { + for (; i < BYTE2HEX_PAD.length; i++) { String str = Integer.toHexString(i); BYTE2HEX_PAD[i] = str; BYTE2HEX_NOPAD[i] = str; @@ -84,8 +85,8 @@ public final class StringUtil { /** * Checks if two strings have the same suffix of specified length * - * @param s string - * @param p string + * @param s string + * @param p string * @param len length of the common suffix * @return true if both s and p are not null and both have the same suffix. Otherwise - false */ @@ -138,7 +139,7 @@ public final class StringUtil { */ public static T toHexStringPadded(T dst, byte[] src, int offset, int length) { final int end = offset + length; - for (int i = offset; i < end; i ++) { + for (int i = offset; i < end; i++) { byteToHexStringPadded(dst, src[i]); } return dst; @@ -198,13 +199,13 @@ public final class StringUtil { int i; // Skip preceding zeroes. - for (i = offset; i < endMinusOne; i ++) { + for (i = offset; i < endMinusOne; i++) { if (src[i] != 0) { break; } } - byteToHexString(dst, src[i ++]); + byteToHexString(dst, src[i++]); int remaining = end - i; toHexStringPadded(dst, src, i, remaining); @@ -244,22 +245,51 @@ public final class StringUtil { * @return {@link CharSequence} the escaped value if necessary, or the value unchanged */ public static CharSequence escapeCsv(CharSequence value) { + return escapeCsv(value, false); + } + + /** + * Escapes the specified value, if necessary according to + * RFC-4180. + * + * @param value The value which will be escaped according to + * RFC-4180 + * @param trimWhiteSpace The value will first be trimmed of its optional white-space characters, + * according to RFC-7230 + * @return {@link CharSequence} the escaped value if necessary, or the value unchanged + */ + public static CharSequence escapeCsv(CharSequence value, boolean trimWhiteSpace) { int length = checkNotNull(value, "value").length(); if (length == 0) { return value; } + + int start = 0; int last = length - 1; - boolean quoted = isDoubleQuote(value.charAt(0)) && isDoubleQuote(value.charAt(last)) && length != 1; + boolean trimmed = false; + if (trimWhiteSpace) { + start = indexOfFirstNonOwsChar(value, length); + if (start == length) { + return EMPTY_STRING; + } + last = indexOfLastNonOwsChar(value, start, length); + trimmed = start > 0 || last < length - 1; + if (trimmed) { + length = last - start + 1; + } + } + + StringBuilder result = new StringBuilder(length + CSV_NUMBER_ESCAPE_CHARACTERS); + boolean quoted = isDoubleQuote(value.charAt(start)) && isDoubleQuote(value.charAt(last)) && length != 1; boolean foundSpecialCharacter = false; boolean escapedDoubleQuote = false; - StringBuilder escaped = new StringBuilder(length + CSV_NUMBER_ESCAPE_CHARACTERS).append(DOUBLE_QUOTE); - for (int i = 0; i < length; i++) { + for (int i = start; i <= last; i++) { char current = value.charAt(i); switch (current) { case DOUBLE_QUOTE: - if (i == 0 || i == last) { + if (i == start || i == last) { if (!quoted) { - escaped.append(DOUBLE_QUOTE); + result.append(DOUBLE_QUOTE); } else { continue; } @@ -267,7 +297,7 @@ public final class StringUtil { boolean isNextCharDoubleQuote = isDoubleQuote(value.charAt(i + 1)); if (!isDoubleQuote(value.charAt(i - 1)) && (!isNextCharDoubleQuote || i + 1 == last)) { - escaped.append(DOUBLE_QUOTE); + result.append(DOUBLE_QUOTE); escapedDoubleQuote = true; } break; @@ -277,10 +307,20 @@ public final class StringUtil { case COMMA: foundSpecialCharacter = true; } - escaped.append(current); + result.append(current); } - return escapedDoubleQuote || foundSpecialCharacter && !quoted ? - escaped.append(DOUBLE_QUOTE) : value; + + if (escapedDoubleQuote || foundSpecialCharacter && !quoted) { + return quote(result); + } + if (trimmed) { + return quoted ? quote(result) : result; + } + return value; + } + + private static StringBuilder quote(StringBuilder builder) { + return builder.insert(0, DOUBLE_QUOTE).append(DOUBLE_QUOTE); } /** @@ -390,7 +430,7 @@ public final class StringUtil { return unescaped; } - /**s + /** * Validate if {@code value} is a valid csv field without double-quotes. * * @throws IllegalArgumentException if {@code value} needs to be encoded with double-quotes. @@ -430,7 +470,8 @@ public final class StringUtil { /** * Find the index of the first non-white space character in {@code s} starting at {@code offset}. - * @param seq The string to search. + * + * @param seq The string to search. * @param offset The offset to start searching at. * @return the index of the first non-white space character or <{@code 0} if none was found. */ @@ -446,6 +487,7 @@ public final class StringUtil { /** * Determine if {@code c} lies within the range of values defined for * Surrogate Code Point. + * * @param c the character to check. * @return {@code true} if {@code c} lies within the range of values defined for * Surrogate Code Point. {@code false} otherwise. @@ -469,4 +511,47 @@ public final class StringUtil { int len = s.length(); return len > 0 && s.charAt(len - 1) == c; } + + /** + * Trim optional white-space characters from the specified value, + * according to RFC-7230. + * + * @param value the value to trim + * @return {@link CharSequence} the trimmed value if necessary, or the value unchanged + */ + public static CharSequence trimOws(CharSequence value) { + final int length = value.length(); + if (length == 0) { + return value; + } + int start = indexOfFirstNonOwsChar(value, length); + int end = indexOfLastNonOwsChar(value, start, length); + return start == 0 && end == length - 1 ? value : value.subSequence(start, end + 1); + } + + /** + * @return {@code length} if no OWS is found. + */ + private static int indexOfFirstNonOwsChar(CharSequence value, int length) { + int i = 0; + while (i < length && isOws(value.charAt(i))) { + i++; + } + return i; + } + + /** + * @return {@code start} if no OWS is found. + */ + private static int indexOfLastNonOwsChar(CharSequence value, int start, int length) { + int i = length - 1; + while (i > start && isOws(value.charAt(i))) { + i--; + } + return i; + } + + private static boolean isOws(char c) { + return c == SPACE || c == TAB; + } } diff --git a/common/src/test/java/io/netty/util/internal/StringUtilTest.java b/common/src/test/java/io/netty/util/internal/StringUtilTest.java index 13016259e0..c8a13f58c0 100644 --- a/common/src/test/java/io/netty/util/internal/StringUtilTest.java +++ b/common/src/test/java/io/netty/util/internal/StringUtilTest.java @@ -19,9 +19,22 @@ import org.junit.Test; import java.util.Arrays; -import static io.netty.util.internal.StringUtil.*; -import static org.hamcrest.CoreMatchers.*; -import static org.junit.Assert.*; +import static io.netty.util.internal.StringUtil.NEWLINE; +import static io.netty.util.internal.StringUtil.commonSuffixOfLength; +import static io.netty.util.internal.StringUtil.simpleClassName; +import static io.netty.util.internal.StringUtil.substringAfter; +import static io.netty.util.internal.StringUtil.toHexString; +import static io.netty.util.internal.StringUtil.toHexStringPadded; +import static io.netty.util.internal.StringUtil.unescapeCsv; +import static io.netty.util.internal.StringUtil.unescapeCsvFields; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; public class StringUtilTest { @@ -331,13 +344,39 @@ public class StringUtilTest { } private static void escapeCsv(CharSequence value, CharSequence expected) { + escapeCsv(value, expected, false); + } + + private static void escapeCsvWithTrimming(CharSequence value, CharSequence expected) { + escapeCsv(value, expected, true); + } + + private static void escapeCsv(CharSequence value, CharSequence expected, boolean trimOws) { CharSequence escapedValue = value; for (int i = 0; i < 10; ++i) { - escapedValue = StringUtil.escapeCsv(escapedValue); + escapedValue = StringUtil.escapeCsv(escapedValue, trimOws); assertEquals(expected, escapedValue.toString()); } } + @Test + public void escapeCsvWithTrimming() { + assertSame("", StringUtil.escapeCsv("", true)); + assertSame("ab", StringUtil.escapeCsv("ab", true)); + + escapeCsvWithTrimming("", ""); + escapeCsvWithTrimming(" \t ", ""); + escapeCsvWithTrimming("ab", "ab"); + escapeCsvWithTrimming("a b", "a b"); + escapeCsvWithTrimming(" \ta \tb", "a \tb"); + escapeCsvWithTrimming("a \tb \t", "a \tb"); + escapeCsvWithTrimming("\t a \tb \t", "a \tb"); + escapeCsvWithTrimming("\"\t a b \"", "\"\t a b \""); + escapeCsvWithTrimming(" \"\t a b \"\t", "\"\t a b \""); + escapeCsvWithTrimming(" testing\t\n ", "\"testing\t\n\""); + escapeCsvWithTrimming("\ttest,ing ", "\"test,ing\""); + } + @Test public void testUnescapeCsv() { assertEquals("", unescapeCsv("")); @@ -465,4 +504,21 @@ public class StringUtilTest { assertFalse(StringUtil.endsWith("-", 'u')); assertFalse(StringUtil.endsWith("u-", 'u')); } + + @Test + public void trimOws() { + assertSame("", StringUtil.trimOws("")); + assertEquals("", StringUtil.trimOws(" \t ")); + assertSame("a", StringUtil.trimOws("a")); + assertEquals("a", StringUtil.trimOws(" a")); + assertEquals("a", StringUtil.trimOws("a ")); + assertEquals("a", StringUtil.trimOws(" a ")); + assertSame("abc", StringUtil.trimOws("abc")); + assertEquals("abc", StringUtil.trimOws("\tabc")); + assertEquals("abc", StringUtil.trimOws("abc\t")); + assertEquals("abc", StringUtil.trimOws("\tabc\t")); + assertSame("a\t b", StringUtil.trimOws("a\t b")); + assertEquals("", StringUtil.trimOws("\t ").toString()); + assertEquals("a b", StringUtil.trimOws("\ta b \t").toString()); + } }