From 52b53af7c6099a41011d3de89fc6fecb4b22422d Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 15:00:10 +0100 Subject: [PATCH 1/4] Do not allow spaces within MediaType's parameters Per https://tools.ietf.org/html/rfc7231#section-3.1.1.1 MediaType can have spaces around parameters pair, but do not allow spaces within parameter. That means that parameter pair forbids spaces around `=` follow up after https://github.com/elastic/elasticsearch/pull/64406#discussion_r517533475 relates #51816 --- .../common/xcontent/ParsedMediaType.java | 15 ++++++++++----- .../common/xcontent/ParsedMediaTypeTests.java | 14 ++++++-------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index 83676cbfa1902..db03a59a8749b 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -63,6 +63,7 @@ public Map getParameters() { /** * Parses a header value into it's parts. + * follows https://tools.ietf.org/html/rfc7231#section-3.1.1.1 but allows only single media type and do not support quality factors * Note: parsing can return null, but it will throw exceptions once https://github.com/elastic/elasticsearch/issues/63080 is done * Do not rely on nulls * @@ -90,15 +91,15 @@ public static ParsedMediaType parseMediaType(String headerValue) { if (paramsAsString.isEmpty()) { continue; } - // intentionally allowing to have spaces around `=` - // https://tools.ietf.org/html/rfc7231#section-3.1.1.1 disallows this - String[] keyValueParam = elements[i].trim().split("="); + //spaces are allowed between parameters, but not between '=' sign + String[] keyValueParam = paramsAsString.split("="); + if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { + throw new IllegalArgumentException("invalid parameters for header [" + headerValue + "]"); + } if (keyValueParam.length == 2) { String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT).trim(); parameters.put(parameterName, parameterValue); - } else { - throw new IllegalArgumentException("invalid parameters for header [" + headerValue + "]"); } } return new ParsedMediaType(splitMediaType[0].trim().toLowerCase(Locale.ROOT), @@ -108,6 +109,10 @@ public static ParsedMediaType parseMediaType(String headerValue) { return null; } + private static boolean hasSpaces(String s) { + return s.trim().equals(s) == false; + } + /** * Resolves this instance to a MediaType instance defined in given MediaTypeRegistry. * Performs validation against parameters. diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java index 15b01014830ba..4656b46998a88 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -81,14 +81,6 @@ public void testWithParameters() { ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123;charset=UTF-8").getParameters()); } - public void testWhiteSpaces() { - //be lenient with white space since it can be really hard to troubleshoot - String mediaType = " application/foo "; - ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + " ; compatible-with = 123 ; charset=UTF-8"); - assertEquals("application/foo", parsedMediaType.mediaTypeWithoutParameters()); - assertEquals((Map.of("charset", "utf-8", "compatible-with", "123")), parsedMediaType.getParameters()); - } - public void testEmptyParams() { String mediaType = "application/foo"; ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + randomFrom("", " ", ";", ";;", ";;;")); @@ -105,6 +97,12 @@ public void testMalformedParameters() { exception = expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; char=set=unknown")); assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo; char=set=unknown]")); + + // do not allow white space in parameters between `=` + exception = expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + " ; compatible-with = 123 ; charset=UTF-8")); + assertThat(exception.getMessage(), + equalTo("invalid parameters for header [application/foo ; compatible-with = 123 ; charset=UTF-8]")); } public void testDefaultAcceptHeader() { From 80b3944f52728bd739fd4ad3f82f52bb014dbffa Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 15:50:07 +0100 Subject: [PATCH 2/4] unnecessary if --- .../elasticsearch/common/xcontent/ParsedMediaType.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index db03a59a8749b..fe96bd847b040 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -96,11 +96,9 @@ public static ParsedMediaType parseMediaType(String headerValue) { if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { throw new IllegalArgumentException("invalid parameters for header [" + headerValue + "]"); } - if (keyValueParam.length == 2) { - String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); - String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT).trim(); - parameters.put(parameterName, parameterValue); - } + String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); + String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT).trim(); + parameters.put(parameterName, parameterValue); } return new ParsedMediaType(splitMediaType[0].trim().toLowerCase(Locale.ROOT), splitMediaType[1].trim().toLowerCase(Locale.ROOT), parameters); From 529869637bd02660aed4d00872078917ef248794 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 16:36:55 +0100 Subject: [PATCH 3/4] remove trimp approach --- .../common/xcontent/ParsedMediaType.java | 9 ++++++--- .../common/xcontent/ParsedMediaTypeTests.java | 11 +++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index fe96bd847b040..d333b2899b6dc 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -93,7 +93,7 @@ public static ParsedMediaType parseMediaType(String headerValue) { } //spaces are allowed between parameters, but not between '=' sign String[] keyValueParam = paramsAsString.split("="); - if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { + if (keyValueParam.length != 2 || hasTrailingSpace(keyValueParam[0]) || hasLeadingSpace(keyValueParam[1])) { throw new IllegalArgumentException("invalid parameters for header [" + headerValue + "]"); } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); @@ -107,10 +107,13 @@ public static ParsedMediaType parseMediaType(String headerValue) { return null; } - private static boolean hasSpaces(String s) { - return s.trim().equals(s) == false; + private static boolean hasTrailingSpace(String s) { + return s.length() == 0 || Character.isWhitespace(s.charAt(s.length()-1)); } + private static boolean hasLeadingSpace(String s) { + return s.length() == 0 || Character.isWhitespace(s.charAt(0)); + } /** * Resolves this instance to a MediaType instance defined in given MediaTypeRegistry. * Performs validation against parameters. diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java index 4656b46998a88..89828c482dfa0 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -103,6 +103,17 @@ public void testMalformedParameters() { () -> ParsedMediaType.parseMediaType(mediaType + " ; compatible-with = 123 ; charset=UTF-8")); assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo ; compatible-with = 123 ; charset=UTF-8]")); + + expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + ";k =y")); + expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + ";k= y")); + expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + ";k = y")); + expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + ";= y")); + expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + ";k=")); } public void testDefaultAcceptHeader() { From fc62ad4fead9f42bc542291380dc1a77091fef93 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 16:40:38 +0100 Subject: [PATCH 4/4] line formatting --- .../common/xcontent/ParsedMediaTypeTests.java | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java index 89828c482dfa0..7b9c5c7a569f0 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -104,16 +104,11 @@ public void testMalformedParameters() { assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo ; compatible-with = 123 ; charset=UTF-8]")); - expectThrows(IllegalArgumentException.class, - () -> ParsedMediaType.parseMediaType(mediaType + ";k =y")); - expectThrows(IllegalArgumentException.class, - () -> ParsedMediaType.parseMediaType(mediaType + ";k= y")); - expectThrows(IllegalArgumentException.class, - () -> ParsedMediaType.parseMediaType(mediaType + ";k = y")); - expectThrows(IllegalArgumentException.class, - () -> ParsedMediaType.parseMediaType(mediaType + ";= y")); - expectThrows(IllegalArgumentException.class, - () -> ParsedMediaType.parseMediaType(mediaType + ";k=")); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + ";k =y")); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + ";k= y")); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + ";k = y")); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + ";= y")); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + ";k=")); } public void testDefaultAcceptHeader() {