You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/26 22:33:14 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7886: Enforce case for well known methods
shinrich opened a new pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886
https://datatracker.ietf.org/doc/html/rfc2616#section-5.1.1 Says “The method is case-sensitive.”
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#issuecomment-859789740
I vote for "not recognized" (501) for mixed case versions of methods that ATS does interact with.
You could make a case either way though, and I would change the PR if others feel strongly about using the other status code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#issuecomment-888659637
Cherry-picked to v9.1.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#issuecomment-856340023
> [RFC 7230 3.1.1. Request Line](https://datatracker.ietf.org/doc/html/rfc7230#section-3.1.1)
> The method token indicates the request method to be performed on the
> target resource. The request method is case-sensitive.
>
> method = token
>
> The request methods defined by this specification can be found in
> Section 4 of [RFC7231], along with information regarding the HTTP
> method registry and considerations for defining new methods.
> ...
> Recipients of an invalid request-line SHOULD respond with either a
> 400 (Bad Request) error or a 301 (Moved Permanently) redirect with
> the request-target properly encoded.
If we think the lower-case or mixed-case method is "invalid request-line", we should send 400 or 301.
> [RFC 7231 4. Request Methods](https://datatracker.ietf.org/doc/html/rfc7231#section-4.1)
> 4.1. Overview
> ...
> By convention, standardized methods are defined in all-uppercase
US-ASCII letters.
...
> The set of methods allowed by a target resource can be listed in an
Allow header field (Section 7.4.1). However, the set of allowed
methods can change dynamically. When a request method is received
that is unrecognized or not implemented by an origin server, the
origin server SHOULD respond with the 501 (Not Implemented) status
code. When a request method is received that is known by an origin
server but not allowed for the target resource, the origin server
SHOULD respond with the 405 (Method Not Allowed) status code.
If we think it's "unrecognized" or "not implemented" method, we SHOULD send 501.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#discussion_r651390093
##########
File path: proxy/hdrs/HdrToken.cc
##########
@@ -548,6 +548,30 @@ hdrtoken_tokenize_dfa(const char *string, int string_len, const char **wks_strin
return wks_idx;
}
+/*-------------------------------------------------------------------------
+ Have to work around that methods are case insensitive while the DFA is
+ case insensitive.
+ -------------------------------------------------------------------------*/
+
+int
+hdrtoken_method_tokenize(const char *string, int string_len)
Review comment:
Note: performance perspective, it looks like this has some overhead, but it might not be matter. Because `GET` is processed in the fast path and doesn't come here.
https://github.com/apache/trafficserver/blob/f002cdbae85509f339f020051d68f8cecdf17bd1/proxy/hdrs/HTTP.cc#L935-L940
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#issuecomment-883739520
You are right it looks like the separate hdrtoken_is_wks is unnecessary. Does save a string compare, but likely pretty short string and not worth the added complexity. Should probably to the comparison in any case. I'll put up another PR to simplify tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#discussion_r640781586
##########
File path: tests/gold_tests/headers/good_request_after_bad.test.py
##########
@@ -0,0 +1,57 @@
+'''
+Verify that request following a ill-formed request is not processed
+'''
+# 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.
+
+import os
+
+Test.Summary = '''
+Verify that request following a ill-formed request is not processed
+'''
+ts = Test.MakeATSProcess("ts", enable_cache=True)
+
+Test.ContinueOnFail = True
+ts.Disk.records_config.update({'proxy.config.diags.debug.tags': 'http',
+ 'proxy.config.diags.debug.enabled': 1
+ })
+
+server = Test.MakeOriginServer("server")
+request_header = {"headers": "GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_header = {
+ "headers": "HTTP/1.1 200 OK\r\nConnection: close\r\nLast-Modified: Tue, 08 May 2018 15:49:41 GMT\r\nCache-Control: max-age=1000\r\n\r\n",
+ "timestamp": "1469733493.993",
+ "body": "xxx"}
+server.addResponse("sessionlog.json", request_header, response_header)
+
+ts.Disk.remap_config.AddLine(
+ 'map / http://127.0.0.1:{0}'.format(server.Variables.Port)
+)
+
+
+# Make a good request to get item in the cache for later tests
+tr = Test.AddTestRun("Good control")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(Test.Processes.ts)
+tr.Processes.Default.Command = 'printf "GET / HTTP/1.1\r\nHost: bob\r\n\r\n" | nc 127.0.0.1 {}'.format(ts.Variables.port)
+tr.Processes.Default.ReturnCode = 0
+
+# Mixed case method name; Should fail but ATS is treating gET as GET
Review comment:
Not with your fix in this patch. I suggest something like:
```
# Methods are case sensitive. Verify that "gET" is not confused with "GET".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7886: Enforce case for well known methods
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7886:
URL: https://github.com/apache/trafficserver/pull/7886#issuecomment-883734396
@shinrich In hdrtoken_method_tokenize(), why does it have to call hdrtoken_is_wks(string) first, when hdtoken_tokenize() does exactly the same thing when you call that ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org