From d4061fcfeb5a959e67547404a1fdfa5a203680db Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 29 Nov 2017 18:56:54 +0100 Subject: [PATCH] Issue #1966 Case Sensitive method (PR #1967) Issue #1966 Case Sensitive method (PR #1967) * Modified the compliance violations to warn if case insensitivety is applied to a header * removed duplicated if * Fixed string comparison * Improved compliance messages and comments * updated expected violation messages * Require a header colon only when in 7230 compliance mode --- .../org/eclipse/jetty/http/HttpParser.java | 41 +++++++++++----- .../eclipse/jetty/http/HttpParserTest.java | 48 +++++++++++++++---- .../servlet/ComplianceViolations2616Test.java | 9 ++-- 3 files changed, 73 insertions(+), 25 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index d2db919ccbb..8dbc5024ea6 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -331,9 +331,10 @@ public class HttpParser } /* ------------------------------------------------------------------------------- */ - protected String legacyString(String orig, String cached) + protected String caseInsensitiveHeader(String orig, String normative) { - return (_compliance!=LEGACY || orig.equals(cached) || complianceViolation(RFC2616,"case sensitive"))?cached:orig; + return (_compliance!=LEGACY || orig.equals(normative) || complianceViolation(RFC2616,"https://tools.ietf.org/html/rfc2616#section-4.2 case sensitive header: "+orig)) + ?normative:orig; } /* ------------------------------------------------------------------------------- */ @@ -646,9 +647,27 @@ public class HttpParser { _length=_string.length(); _methodString=takeString(); + + // TODO #1966 This cache lookup is case insensitive when it should be case sensitive by RFC2616, RFC7230 HttpMethod method=HttpMethod.CACHE.get(_methodString); if (method!=null) - _methodString=legacyString(_methodString,method.asString()); + { + switch(_compliance) + { + case LEGACY: + // Legacy correctly allows case sensitive header; + break; + + case RFC2616: + case RFC7230: + if (!method.asString().equals(_methodString) && _complianceHandler!=null) + _complianceHandler.onComplianceViolation(_compliance,HttpCompliance.LEGACY, + "https://tools.ietf.org/html/rfc7230#section-3.1.1 case insensitive method "+_methodString); + // TODO Good to used cached version for faster equals checking, but breaks case sensitivity because cache is insensitive + _methodString = method.asString(); + break; + } + } setState(State.SPACE1); } else if (b < SPACE) @@ -749,7 +768,7 @@ public class HttpParser else if (b < HttpTokens.SPACE && b>=0) { // HTTP/0.9 - if (complianceViolation(RFC7230,"HTTP/0.9")) + if (complianceViolation(RFC7230,"https://tools.ietf.org/html/rfc7230#appendix-A.2 HTTP/0.9")) throw new BadMessageException("HTTP/0.9 not supported"); handle=_requestHandler.startRequest(_methodString,_uri.toString(), HttpVersion.HTTP_0_9); setState(State.END); @@ -816,7 +835,7 @@ public class HttpParser else { // HTTP/0.9 - if (complianceViolation(RFC7230,"HTTP/0.9")) + if (complianceViolation(RFC7230,"https://tools.ietf.org/html/rfc7230#appendix-A.2 HTTP/0.9")) throw new BadMessageException("HTTP/0.9 not supported"); handle=_requestHandler.startRequest(_methodString,_uri.toString(), HttpVersion.HTTP_0_9); @@ -934,7 +953,7 @@ public class HttpParser _host=true; if (!(_field instanceof HostPortHttpField) && _valueString!=null && !_valueString.isEmpty()) { - _field=new HostPortHttpField(_header,legacyString(_headerString,_header.asString()),_valueString); + _field=new HostPortHttpField(_header,caseInsensitiveHeader(_headerString,_header.asString()),_valueString); add_to_connection_trie=_fieldCache!=null; } break; @@ -963,7 +982,7 @@ public class HttpParser if (add_to_connection_trie && !_fieldCache.isFull() && _header!=null && _valueString!=null) { if (_field==null) - _field=new HttpField(_header,legacyString(_headerString,_header.asString()),_valueString); + _field=new HttpField(_header,caseInsensitiveHeader(_headerString,_header.asString()),_valueString); _fieldCache.put(_field); } } @@ -1031,7 +1050,7 @@ public class HttpParser case HttpTokens.SPACE: case HttpTokens.TAB: { - if (complianceViolation(RFC7230,"header folding")) + if (complianceViolation(RFC7230,"https://tools.ietf.org/html/rfc7230#section-3.2.4 folding")) throw new BadMessageException(HttpStatus.BAD_REQUEST_400,"Header Folding"); // header value without name - continuation? @@ -1154,13 +1173,13 @@ public class HttpParser { // Have to get the fields exactly from the buffer to match case String fn=field.getName(); - n=legacyString(BufferUtil.toString(buffer,buffer.position()-1,fn.length(),StandardCharsets.US_ASCII),fn); + n=caseInsensitiveHeader(BufferUtil.toString(buffer,buffer.position()-1,fn.length(),StandardCharsets.US_ASCII),fn); String fv=field.getValue(); if (fv==null) v=null; else { - v=legacyString(BufferUtil.toString(buffer,buffer.position()+fn.length()+1,fv.length(),StandardCharsets.ISO_8859_1),fv); + v=caseInsensitiveHeader(BufferUtil.toString(buffer,buffer.position()+fn.length()+1,fv.length(),StandardCharsets.ISO_8859_1),fv); field=new HttpField(field.getHeader(),n,v); } } @@ -1253,7 +1272,7 @@ public class HttpParser break; } - if (b==HttpTokens.LINE_FEED && !complianceViolation(RFC7230,"name only header")) + if (b==HttpTokens.LINE_FEED && !complianceViolation(RFC7230,"https://tools.ietf.org/html/rfc7230#section-3.2 No colon")) { if (_headerString==null) { diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java index b2d65e39d2a..a049c3cd91d 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java @@ -350,7 +350,7 @@ public class HttpParserTest } @Test - public void testNoColon2616() throws Exception + public void testNoColonLegacy() throws Exception { ByteBuffer buffer = BufferUtil.toBuffer( "GET / HTTP/1.0\r\n" + @@ -360,7 +360,7 @@ public class HttpParserTest "\r\n"); HttpParser.RequestHandler handler = new Handler(); - HttpParser parser = new HttpParser(handler,HttpCompliance.RFC2616); + HttpParser parser = new HttpParser(handler,HttpCompliance.LEGACY); parseAll(parser, buffer); Assert.assertTrue(_headerCompleted); @@ -375,7 +375,7 @@ public class HttpParserTest Assert.assertEquals("Other", _hdr[2]); Assert.assertEquals("value", _val[2]); Assert.assertEquals(2, _headers); - Assert.assertThat(_complianceViolation, Matchers.containsString("name only")); + Assert.assertThat(_complianceViolation, Matchers.containsString("No colon")); } @Test @@ -674,10 +674,42 @@ public class HttpParserTest } @Test - public void testCaseInsensitive() throws Exception + public void testCaseSensitiveMethod() throws Exception { ByteBuffer buffer = BufferUtil.toBuffer( - "get / http/1.0\r\n" + + "gEt / http/1.0\r\n" + + "Host: localhost\r\n" + + "Connection: close\r\n" + + "\r\n"); + HttpParser.RequestHandler handler = new Handler(); + HttpParser parser = new HttpParser(handler, -1, HttpCompliance.RFC7230); + parseAll(parser, buffer); + Assert.assertNull(_bad); + Assert.assertEquals("GET", _methodOrVersion); + Assert.assertThat(_complianceViolation, Matchers.containsString("case insensitive method gEt")); + } + + @Test + public void testCaseSensitiveMethodLegacy() throws Exception + { + ByteBuffer buffer = BufferUtil.toBuffer( + "gEt / http/1.0\r\n" + + "Host: localhost\r\n" + + "Connection: close\r\n" + + "\r\n"); + HttpParser.RequestHandler handler = new Handler(); + HttpParser parser = new HttpParser(handler, -1, HttpCompliance.LEGACY); + parseAll(parser, buffer); + Assert.assertNull(_bad); + Assert.assertEquals("gEt", _methodOrVersion); + Assert.assertNull(_complianceViolation); + } + + @Test + public void testCaseInsensitiveHeader() throws Exception + { + ByteBuffer buffer = BufferUtil.toBuffer( + "GET / http/1.0\r\n" + "HOST: localhost\r\n" + "cOnNeCtIoN: ClOsE\r\n" + "\r\n"); @@ -697,10 +729,10 @@ public class HttpParserTest } @Test - public void testCaseSensitiveLegacy() throws Exception + public void testCaseInSensitiveHeaderLegacy() throws Exception { ByteBuffer buffer = BufferUtil.toBuffer( - "gEt / http/1.0\r\n" + + "GET / http/1.0\r\n" + "HOST: localhost\r\n" + "cOnNeCtIoN: ClOsE\r\n" + "\r\n"); @@ -708,7 +740,7 @@ public class HttpParserTest HttpParser parser = new HttpParser(handler, -1, HttpCompliance.LEGACY); parseAll(parser, buffer); Assert.assertNull(_bad); - Assert.assertEquals("gEt", _methodOrVersion); + Assert.assertEquals("GET", _methodOrVersion); Assert.assertEquals("/", _uriOrStatus); Assert.assertEquals("HTTP/1.0", _versionOrReason); Assert.assertEquals("HOST", _hdr[0]); diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ComplianceViolations2616Test.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ComplianceViolations2616Test.java index 68e72bf3084..6b805040edb 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ComplianceViolations2616Test.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ComplianceViolations2616Test.java @@ -147,8 +147,7 @@ public class ComplianceViolations2616Test String response = connector.getResponse(req1.toString()); assertThat("Response status", response, containsString("HTTP/1.1 200 OK")); - assertThat("Response headers", response, containsString("X-Http-Violation-0: RFC2616