Fix for issue #21299. Change HEAD action mapping to GET action interface and add HEAD request handling #21378
Conversation
…404 responses to HEAD requests.
|
|
|
Hi @mattijv. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
|
@Nazar65 #21331 is not a fix to the underlying issue, it just makes one specific controller accept HEAD requests. A lot of other routes still wont (e.g. /customer/account/login). The real issue is that Magento caches a 404 response for HEAD requests. I could see it argued that all routes responding to GET requests should also respond to a HEAD request, but then the issue lies with how the FrontController decides whether a controller can handle a request or not. If the core team decides that all GET routes should also respond to a HEAD request, then the interface mapping should be changed. |
|
Hi @mattijv and @Nazar65, |
|
@ihor-sviziev Thank you for your response. I have couple points I'd like to make, for your consideration. To me it seems the issue is one of the two here:
or
In case you determine the problem to be number 1, then my pull request is not correct and the real fault lies in how the different requests are mapped to different intefaces in app/etc/di.xml. HEAD and GET should (in my opinion) both be mapped to the same interface. If on the other hand you decide that routes should be able to respond to GET requests and ignore HEAD requests, then my pull request might be a way to fix the issue. No offence meant, but in neither case does #21331 seem like the correct solution, as it is only making one affected controller respond to HEAD requests. This won't fix the issue for several other controllers that respond to GET requests, but not HEAD. That said, I trust that a correct solution is implemented for this issue. |
|
@mattijv did you find why this issue not present in 2.2-develop ? just checked on 2.2-dev and works all perfectly. Maybe we need find why this works on 2.2-dev ? |
|
@Nazar65 The problem doesn’t exist in M2.2 because M2.3 added validation that checks if a controller responds to a certain type of HTTP request (GET, HEAD, POST etc). In M2.2 GET and HEAD requests were treated the same, but now a controller must specify that it will respond to HEAD requests. |
|
@mattijv with your solution we always get 404 with curl i think this is not correct, if i do request like |
|
@Nazar65 Yes, like I stated in #21378 (comment), there are two ways this can be fixed. Either the routes SHOULD respond to HEAD requests, in which case my fix is wrong, or they can respond with a 404 to a HEAD request but 200 to a GET, in which case my fix should be something close to what should be implemented. It's up to the core team to decide if GET routes should automatically respond to a HEAD request. |
There was a problem hiding this comment.
Hi @mattijv,
We discussed this issue with Architects and here is result: #21299 (comment)
Could you update your PR based on following comment?
|
@ihor-sviziev Thank you for the message. I wrote some thoughts in that thread, but would be happy to update this PR if my proposed changes are deemed a suitable solution. |
…ce and are sent without body attached.
…t cache 404 responses to HEAD requests." This reverts commit 46191d8.
|
@ihor-sviziev I've updated the pull request based on conversations here: #21299 (comment) The changes most likely require review. The critical change for fixing the original issue is the change of the HEAD request map in the app/etc/di.xml to the GET request interface. I've left the now useless HttpHeadActionInterface for backwards compatibility, but marked it deprecated (hopefully I got that right). I also added code to the framework that strips the body from HEAD requests as this was not present before (the HEAD requests were responded to with the full body). |
| */ | ||
| private function handleHeadRequest() | ||
| { | ||
| $contentLength = strlen($this->_response->getContent()); |
There was a problem hiding this comment.
The strlen function isn’t binary safe (i.e. images and compressed content) and also doesn’t handle multi-byte characters. I‘m not sure about the proper things to do here, but it would be a good idea to do some more research and maybe add some tests for non-ascii response bodies.
There was a problem hiding this comment.
I was under the assumption that strlen specifically returns the number of bytes in the string, instead of the number of characters.[1] Isn't that what the Content-Length header should contain?
[1] http://php.net/manual/en/function.strlen.php#refsect1-function.strlen-notes
There was a problem hiding this comment.
I do agree on adding non-ascii test cases, that is a good idea.
There was a problem hiding this comment.
On further research it seems that some installations of PHP might have overloaded the strlen to use mb_strlen instead.[1] This would break the content length on multibyte characters as they would be counted as one, instead of the actual number of bytes.
Safer way seems to be to call mb_strlen explicitly with encoding that only supports byte characters.[2] This way we should always get the correct byte count for the body contents.
[1] http://php.net/manual/en/mbstring.overload.php
[2] https://stackoverflow.com/a/9718273
There was a problem hiding this comment.
I guess the important thing to check for would be if it is null byte safe \0. That might be PHP version dependent, hopefully PHP7 does not rely on low level C strings. Otherwise your suggested use of mb_strlen sounds good to me.
There was a problem hiding this comment.
As far as I can see, the null byte is treated as a one byte character. It won't terminate strings prematurely.[1]
There was a problem hiding this comment.
Updated the pull request to use mb_strlen instead of strlen and added test cases for some potentially anomalous content.
…ontent length is calculated in bytes in all PHP installations. Added test cases to cover non-ascii content (multi-byte characters, null byte, LTR text).
|
Hi @sidolov, thank you for the review. |
ghost
added
Progress: testing in progress
and removed
Progress: ready for testing
labels
|
✔️ QA Passed Before:
After:
|
ghost
added
Progress: extended testing
and removed
Progress: testing in progress
labels
|
Thank you @o-iegorov for fixing the issues pointed out by @joni-jones. I was unfortunately unable to find the time to work on them. |
|
I may be speaking out of turn here, but I created a composer patch from this PR diff, and the problem still exists. After successful deployment on 2.3.1, and verifying the patch was applied properly I flushed the magento cache. Then ran |
|
@SAN1TAR1UM The actual fix for the 404s in this PR is the change to the app/etc/di.xml file. The PHP changes are mostly to handle the HEAD requests per spec. Can you make sure that the composer patch applied the changes to the app/etc/di.xml file? |
|
@SAN1TAR1UM Just to confirm that the fix should work in M2.3.1 I made a clean install and changed the HEAD action mapping in app/etc/di.xml. Before the change the site was returning 404s on curl -I but after the change it correctly returned 200s. |
| @@ -57,6 +74,9 @@ public function testSend(): void | |||
| ->method('getMimeType') | |||
| ->with($file) | |||
| ->will($this->returnValue($contentType)); | |||
| $this->request->expects($this->once()) | |||
| ->method('isHead') | |||
| ->will($this->returnValue(false)); | |||
There was a problem hiding this comment.
Please, use $this->willReturnValue instead.
| $this->frontControllerMock->expects($this->once()) | ||
| ->method('dispatch') | ||
| ->with($this->requestMock)->will( | ||
| $this->returnCallback( |
There was a problem hiding this comment.
Please, use willReturnCallback or willThrowException instead
| */ | ||
| public function __construct( | ||
| \Magento\Framework\HTTP\PhpEnvironment\Response $response, | ||
| \Magento\Framework\File\Mime $mime | ||
| \Magento\Framework\File\Mime $mime, | ||
| \Magento\Framework\App\Request\Http $request = null |
There was a problem hiding this comment.
Please, avoid usage of FQCN
|
@mattijv I confirmed the patch ran according to the jenkins logs, but upon review of the di.xml file it looks like the change is not there. Looks like I have a deployment issue, I will rectify it and re-test and advise. |
|
Looks like composer install in a fresh directory overwrites the app/etc/di.xml file from the code repository. Does anyone know which composer dep is making this change? |
…nto 2.3-develop-mattijv
… interface and add HEAD request handling
… interface and add HEAD request handling
|
@SAN1TAR1UM: It's best not to change the file Hope this helps :) |
|
Hi @mattijv, thank you for your contribution! |
…action interface and add HEAD request handling #21378
Description
M2.3 changed the FrontController to explicitly require actions to specify if they respond to HEAD requests. Magento was then caching the 404 response resulting from these HEAD requests and serving them to subsequent GET requests even if they normally would have resulted in 200 response.
Fixed Issues
Manual testing scenarios
curl -I HEAD http://magento.local/customer/account/login/)Expected result
Customer login page
Actual result
404 page
Contribution checklist