Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HEAD request returns 404 #21299

Closed
kolucciy opened this issue Feb 18, 2019 · 35 comments

Comments

@kolucciy
Copy link

commented Feb 18, 2019

Preconditions (*)

  1. Magento 2.3.0
  2. HTTPS
  3. Nginx

Steps to reproduce (*)

  1. Clear FPC and don't access the site
  2. Run curl -X HEAD -i https://demo1-m2.mage.direct/ or curl -I https://demo1-m2.mage.direct/
  3. After couple of HEAD requests with response 404 the home page gets cached and after that any valid GET request returns 404 until FPC is refreshed.

Expected result (*)

  1. I expect HEAD request to return 200 and not 404

Actual result (*)

  1. HEAD request returns 404
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Hi @kolucciy. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@kolucciy do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@engcom-backlog-nazar engcom-backlog-nazar self-assigned this Feb 18, 2019

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Hi @engcom-backlog-nazar. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 5. Verify that the issue is reproducible on 2.2-develop branch.

    Details- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x

  • Next steps are available in case you are a member of Community Maintainers.

  • 6. Add label Issue: Confirmed once verification is complete.

  • 7. Make sure that automatic system confirms that report has been added to the backlog.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

Confirmed by @engcom-backlog-nazar
Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-98314 were created

Issue Available: @engcom-backlog-nazar, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

Jitheesh pushed a commit to Jitheesh/magento2 that referenced this issue Feb 19, 2019

Fix for bug HEAD request returns 404 magento#21299
- CMS index action is not handling head requests

@Jitheesh Jitheesh referenced this issue Feb 19, 2019

Closed

Fix for bug HEAD request returns 404 #21299 #21331

0 of 1 task complete

Jitheesh added a commit to Jitheesh/magento2 that referenced this issue Feb 20, 2019

Fix for bug HEAD request returns 404 magento#21299
- CMS index action is not handling head requests
@mattijv

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

This is an issue with the Magento built-in full page cache.
Here's a quick fix for the issue while waiting for Magento to fix the root cause in the core:

Add this plugin in a di.xml somewhere

<type name="Magento\Framework\App\PageCache\Kernel">
    <plugin name="prevent_caching_404_head_results" type="VENDOR\MODULE\Plugin\PreventCachingHead404ResultsPlugin" />
</type>

And create the following class in the path defined by the di.xml file

<?php

namespace VENDOR\MODULE\Plugin;

class PreventCachingHead404ResultsPlugin
{
    /**
     * @var \Magento\Framework\App\Request\Http
     */
    protected $request;

    /**
     * PreventCachingHeadRequestsPlugin constructor.
     * @param \Magento\Framework\App\Request\Http $request
     */
    public function __construct(\Magento\Framework\App\Request\Http $request)
    {
        $this->request = $request;
    }

    /**
     * Prevent caching HEAD requests that return a 404 result
     * @param \Magento\Framework\App\PageCache\Kernel $subject
     * @param callable $proceed
     * @param \Magento\Framework\App\Response\Http $response
     * @return void
     */
    public function aroundProcess(
        \Magento\Framework\App\PageCache\Kernel $subject,
        callable $proceed,
        \Magento\Framework\App\Response\Http $response
    ) {
        if ($this->request->isHead() && $response->getHttpResponseCode() == 404) {
            return;
        }
        return $proceed($response);
    }
}

Alternatively you can disable the fullpage cache, but that is not advisable since it affects performance. Better solution is to put the store behind a Varnish cache, since that seems to be unaffected by this.

@buskamuza

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

HEAD requests should be supported automatically (on the framework level) for all GET requests, just w/o body. Caching should take this into account (GET should have separate cache to avoid empty body).

Looks like also supported by @Vinai

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Yes, HEAD requests should be cached, and fixing only the index action is bandaid.
Instead all GET actions should also respond to HEAD requests automatically but without the body.
First idea would be an after plugin with a higher sort order than the PageCache plugins for \Magento\Framework\App\FrontControllerInterface::dispatch. The new plugin inspects the request method, and for HEAD requests it always returns an instance of a new result implementation HttpHeadResult. That response type takes an existing result and the response and copies all headers that are currently set. When \Magento\Framework\Controller\ResultInterface::renderResult is called it set's an empty string on the response however.

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@buskamuza I'm wondering if there really needs to be a separate cache for HEAD requests?

Since the same controller is returning the result for both HEAD and GET requests (and I think it's infeasible to make the controller return different results based on if the request is HEAD or GET), I don't think there's a need for a separate cache. The framework should just send the HEAD part of the result back, instead of the whole result (which it might already do anyway?).

@Vinai IMO there's no need for a plugin. I think we could simply map the HEAD requests to the GET action interface in app/etc/di.xml (the arguments for Magento\Framework\App\Request\HttpMethodMap) and completely remove the Magento\Framework\App\Action\HttpHeadActionInterface interface. Unless there's some specific reason to have the HEAD interface existing separately, I feel that removing that interface completely and defining the GET interface to be the action for both is a cleaner solution. The framework would then handle the two different request types as I outlined above.

Thoughts on this suggestion?

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@mattijv The change you describe introduces a pretty big backward incompatible change. Also it doesn’t address the the issue of the response body, which should be empty for propper HTTP HEAD request handling.

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@Vinai I'm not sure what the backwards incompatible change would be? I assume you mean removing the HttpHeadActionInterface? It is true that if some third party developer has implemented that Interface in their code, removing it would break their code. And I guess if they have chosen to implement a route that only responds to HEAD requests, that would be broken by the method mapping change.

The first issue could be avoided by leaving the interface in the codebase. The latter is something that would be hard to reconcile with my proposed changes, but is likely a rare usecase and an improper way to handle HTTP requests anyway...

Is there some other backwards incompatibility the change would cause that I'm missing? Since the action interfaces were introduced in M2.3, I feel like the real world impact of changing the mappings at this stage would be small. I would rather have the codebase be stripped from unnecessary parts, if possible.

I see now what you intented the plugin to do. I was under the assumption that the zend framework would handle stripping the body from the response, but that was a mistake on my part. Apologies. Yes, a plugin like that is needed to handle the HEAD responses correctly.

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Yes, removing an interface is a backward incompatible change.
Also, removing it from classes that previously implemented it also is a bc break (although I haven’t checked if any class in M2 currently implements it).

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@Vinai @buskamuza After some digging around in the parts of Magento internals that I wasn't too familiar with, it seems there are two different ways the HEAD requests could be handled.

  1. Treat the HEAD requests internally the same as GET requests (i.e. use the same rendering methods and caches) and have a plugin attached to (e.g.) Magento\Framework\HTTP\PhpEnvironment\Response sendResponse method that clears the body for all HEAD requests. This way the internal pipeline would be simpler but HEAD requests would have some unnecessary overhead from rendering the body (which would be cached so the work would not be completely wasted).

  2. Change the result of the HEAD request to a different class with only the headers and an empty body. This would then require a separate cache to avoid the empty body issue.

Option 1 seems to be easier to implement and would not introduce internal complexity to the core framework, but possibly has some overhead tradeoffs. To me option 1 feels like it better follows the philosophy of GET and HEAD requests, but I might be mistaken.

Option 2 would be a more complex change, but would potentially result in faster responses to HEAD requests (unless there is a possibility of rendering a result affecting the headers of said result, in which case the rendering is anyway necessary?).

Anyway, this decision is above my paygrade, so to speak. And I might very well have overlooked some issues.

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Also, removing it from classes that previously implemented it also is a bc break (although I haven’t checked if any class in M2 currently implements it).

@Vinai In M2.3 I can find any class that implements it currently and it isn't present in earlier releases, which is why I think removing it at this stage shouldn't be a major problem. But then again I don't have to support thousands of 3rd party developers...

IMO implementing it as a separate interface feels like a mistake and this would be the best time to fix the issue as M2.3 has been out for a fairly short time and the use case for that interface seems rare anyway.

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

The response content has to be generated for HEAD requests so the Content-Length HTTP header can be correctly set, which is an important use case for HEAD requests.
The Content needs to be generated, the Content-Length header needs to be set, and then the content body should be removed from the response.

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@Vinai Of course, silly of me to overlook that. In that case I would suggest changes similar to the option 1 I outlined above. If you feel that removing the interface is too major of a change, I would still propose changing the interface mappings so that both HEAD and GET map to the same interface, and leaving the HttpHeadActionInterface for backwards compatibility (even if it is unused).

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

On further inscpection the controller_front_send_response_before event seems to be an ideal place to modify the response for HEAD requests, since it receives both request and response as the event params.

I also noticed that at the moment all static resources also return the full body for HEAD requests. Perhaps the Magento\Framework\App\StaticResouce could emit a similar event for the same purpose?

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

According to the Magento Technical Guidelines an event (including its arguments) must not be modified, a plugin must be used instead (see paragraph 14 at https://devdocs.magento.com/guides/v2.3/coding-standards/technical-guidelines.html)

I’m not saying I agree with that, but any solution in the core should probably take that into account.

@Vinai

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

I don’t think it should take a lot to fix the built in page cache to distinguish the GET and HEAD requests and cache them separately.
That way it behaves correctly.

@LiamKarlMitchell

This comment has been minimized.

Copy link

commented Apr 3, 2019

Have a production mode 2.3 site with Nginx that sometimes serves homepage as a 404 think this might be why. Suspect head requests sent by a "down notifier" service and then the 404 is cached and served up by full page cache.

Confirm 404 when doing Head request, flushing FPC cleans it out.

jQuery.ajax({ type: "HEAD", async: true, url: '/', }).done(function(message,text,jqXHR){ console.log(message, text, jqXHR.status) });

Someone made a plugin module to work around this in the meantime, same as mattijv's earlier post.
https://github.com/magic42/module-prevent-head-404-cache

o-iegorov added a commit to mattijv/magento2 that referenced this issue Apr 15, 2019

o-iegorov added a commit to mattijv/magento2 that referenced this issue Apr 18, 2019

o-iegorov added a commit to mattijv/magento2 that referenced this issue Apr 18, 2019

@m2-backlog m2-backlog bot moved this from PR In Progress to Done in Community Backlog Apr 20, 2019

magento-engcom-team added a commit that referenced this issue Apr 20, 2019

ENGCOM-4474: Fix for issue #21299. Change HEAD action mapping to GET …
…action interface and add HEAD request handling #21378

 - Merge Pull Request #21378 from mattijv/magento2:2.3-develop
 - Merged commits:
   1. 46191d8
   2. d8eb160
   3. 72630cd
   4. 4f7db3a
   5. e198914
   6. 22b9e56
   7. 69bd2dd
   8. ce2e4d3
   9. 56d4cc2
   10. 2b58789
   11. 5b79a30
   12. 6bac208
   13. 880404c
   14. bc3ef7a
   15. be77120
   16. fecce53
   17. 3d19c09
   18. 2a337f9
   19. b5683e8
   20. 740e4bd

magento-engcom-team pushed a commit that referenced this issue Apr 20, 2019

ENGCOM-4474: Fix for issue #21299. Change HEAD action mapping to GET …
…action interface and add HEAD request handling #21378
@SteveEdson

This comment has been minimized.

Copy link

commented May 29, 2019

Any idea when 2.3.2 will be released? Could do with a fix for this being available.

Thanks

@mattijv

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@SteveEdson While waiting for the next release you can do a couple interim fixes:

  1. Setup a Varnish cache infront of your Magento installation and use that instead of the Magento built-in full page cache. Running a Varnish cache is in any case beneficial.

and/or

  1. Apply the change in this commit to the app/etc/di.xml file. That is the most important change to prevent HEAD requests from 404'ing your controllers (the rest is just fluff about handling the HEAD requests up to spec).
@sprankhub

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@mattijv thanks for your work on that. Option 2 does not work for me on Magento 2.3.1. I tried to apply your changes from the pull request via cweagans/composer-patches, which worked after some time (by removing the changes made to test classes and applying the app/etc/di.xml change manually). However, the bug is still present. So either:

  1. I am not applying the changes correctly.
  2. Other unreleased changes are required for the fix, which I do not have in 2.3.1.
  3. Your fix does not work correctly.

May I re-use this issue to test this on a new 2.3-develop instance or should I create a new issue?

@sprankhub

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@magento give me 2.3-develop instance

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Hi @sprankhub. Thank you for your request. I'm working on Magento 2.3-develop instance for you

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Hi @sprankhub, here is your Magento instance.
Admin access: https://i-21299-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@sprankhub

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Indeed, this works on 2.3-develop. So either option 1 or 2 is correct. Then I will need to wait for 2.3.2...

@mattijv

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@sprankhub In practice only the app/etc/di.xml change should be needed as the root cause is the missing head request interface in most controllers, which leads to Magento deciding that there is no controller that can respond to the incoming request. The other changes are tests or related to stripping body content from the head responses.

Make sure that your change persists and that you apply the change before running the deployment commands. If the problem persists despite that, maybe you are running into some other (related?) issue?

@sprankhub

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Got it. I only patched app/etc/di.xml, but now I understood that I (also?) have to patch the file vendor/magento/magento2-base/app/etc/di.xml. Thanks @mattijv!

@HugoGerritsen

This comment has been minimized.

Copy link

commented Jun 13, 2019

I am experiencing exactly the same with the 404 error that shows randomly after a while. I did the same as sprankhub, but am also experiencing 429 errors on my robots and sitemap. Could this also be related to this issue? Hope this is resolved soon.

@HugoGerritsen

This comment has been minimized.

Copy link

commented Jun 14, 2019

I changed the di.xml, but it is still occuring. Can you arrange a fix for me?

@sprankhub

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@HugoGerritsen are you talking about 404 errors or 429 errors (probably a different issue)? Did you patch both di.xml files and clear the cache?

@HugoGerritsen

This comment has been minimized.

Copy link

commented Jun 15, 2019

@sprankhub I’m talking about the 404 errors. I updates both di.xml files and the error still comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.