codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.3k stars 1.9k forks source link

Bug: IsAJAX() relies on inconsistent headers #2454

Closed MGatner closed 4 years ago

MGatner commented 4 years ago

Newer JavaScript implementations (i.e. fetch) don't set the HTTP_X_REQUESTED_WITH header, making the current implementation of IncomingRequest::isAJAX() unreliable.

So far I have not found a reliable way to distinguish fetch requests, and if this is ultimately the case then isAJAX() should be removed or reworked so it is clear that it is not definitive.

Ref. https://stackoverflow.com/questions/44202593/detect-a-fetch-request-in-php

MGatner commented 4 years ago

A temporary workaround is to add the header onto the fetch call, if you're writing it yourself:

fetch(url, {
    method: "get",
    headers: {
      "Content-Type": "application/json",
      "X-Requested-With": "XMLHttpRequest"
    }
lonnieezell commented 4 years ago

Oh, well that's a giant PITA.

MGatner commented 4 years ago

Right? With so many things moving to "pure" JavaScript fetch is now becoming a standard. I pretty much gave up, looks like there is no way to differentiate fetch calls unless the developer specifies it. I'd be curious to see how other frameworks are dealing with this.

lonnieezell commented 4 years ago

Did a quick search and didn't find any other ways. So I guess the right thing to do here is to update the user guide to tell them it is not reliable, and provide an example for how to send the header along with their requests.

I'm tempted to rip it out altogether, but if they send the header it still works, and still works with "legacy" javascript solutions, like jQuery, etc. so probably not worth destroying yet. And I know people are already using the framework...

MGatner commented 4 years ago

Is it too gross to change it to isXMLHttpRequest()? That makes it clear that it doesn't cover all AJAX requests but it will catch instances with the header set.

lonnieezell commented 4 years ago

I think that's gross yes. :) But I also don't get too caught up on the name, because much of what we do is not "Asynchronous JavaScript and XML" anymore anyway. AJAX has become the umbrella term for that so I think it is still relevant and the most common name developers use.

Oh - and I don't hardly ever use it as an "XMLHttpRequest" either. More a "JSONHttpRequest" :P

MGatner commented 4 years ago

Oh definitely, but I was referring to the header setting. Right now isAJAX() is pretty much if ($headers['X-Requested-With'] == 'XMLHttpRequest') so naming it after what the check actually is would make it clear what it does and doesn't do.

lonnieezell commented 4 years ago

Yeah, I get that. I don't think it's necessary as long as there's a big section in the docs that addresses this.

I think it definitely warrants looking at how others are handling it (if at all), and a little more research before final release.

jlamim commented 4 years ago

I think it's important to have this information in the documentation so that everyone who is already familiar with this new version of the framework can know about this issue related to how the IsAJAX () method works when used with pure JavaScript.

If you wish I can organize the information added here in this issue and insert this information into the IncomingRequest class documentation.

lonnieezell commented 4 years ago

@jlamim that would be great. I think we need the information on its own page, since it seems it would be referenced from a few different places, including Controller Filters, and Routing maybe?

jlamim commented 4 years ago

@lonnieezell when you said "own page" did you mean that it would be better to post to my personal page / blog or the respective method page within the documentation?

If it's a specific page of the documentation, which page do you recommend?

lonnieezell commented 4 years ago

@jlamim I mean we should create a new page in the documentation specifically about AJAX calls that can be referenced from the 3-4 places it should be mentioned.

jlamim commented 4 years ago

Now it is clear to me. I think it is very valid for us to adopt this strategy regarding AJAX content with CodeIgniter 4. It will make it much easier for the community, as this feature tends to be widely used now that it is easier to integrate CodeIgniter with Vue, Angular and React.

jlamim commented 4 years ago

@lonnieezell and @MGatner what do you think about creating a new topic in the Tutorials section, with this information and example, until we can find an efficient and easy-to-understand way to address the issue within the framework?

najdanovicivan commented 4 years ago

@lonnieezell and @MGatner what do you thing about adding config for is_ajax so that it is possible to set which headers should be picked up to state that request is ajax something like this

$ajax_headers = [
    ["X-Requested-With" => ""XMLHttpRequest"],
    [
        "Content-Type" => "application/json",
        "X-Requested-With" => "JSONHttpRequest",
    ],
];

With this sample config requests which contain "X-Requested-With": "XMLHttpRequest" or both "Content-Type" : "application/json" and "X-Requested-With" : "JSONHttpRequest" will be considered as ajax requests.

MGatner commented 4 years ago

The problem is that fetch requests send absolutely no distinguishing headers over regular browser requests (unless the developer chooses to add them). I think isAjax() is becoming a dated function and endpoints that need to differentiate request behavior will have to specify that as part of their definition.

MGatner commented 4 years ago

FWIW Laravel's Request object has a function ajax() that is actually just a wrapper for Symfony's isXmlHttpRequest(). Both a simple checks against the X-Requested-With header and will fail to the same scenarios as ours.

Ref. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1707

lonnieezell commented 4 years ago

Makes sense. So many people use Javascript frameworks that automatically insert that so it doesn't get as noticed, I don't think. But without more pure javascript usage and especially raw fetch, on the rise, it will be more problematic. I think the best we can do is document it.

jlamim commented 4 years ago

I agree with @lonnieezell . With so many variations, the best we have to do at the moment is to define a way of working with the IsAJAX () method that works for the largest possible number of requests and in the official documentation we put more details on these differences in relation to the X-Requested-With. Then we follow and see the feedbacks and from them we think of other forms of implementation that make the method even more efficient.

element-code commented 3 years ago

Sorry for digging in the Graves but I stumbled over the documented way to use this and thought it would be better to:

This has the following advantages:


If deprecating the isAJAX functionality is not an option:

MGatner commented 3 years ago

@element-code I'm definitely open. I suspect that the current isAjax headers will only get less common, and moving toward a proper implementation is a good idea. Would you like to start a PR with some of the content negotiation handling you mentioned? There's a lot happening with HTTP/API components right now so it would be a good moment for a push in that area.

MGatner commented 3 years ago

Pulling in @caswell-cs in case of overlap

element-code commented 3 years ago

What also needs to be thought of is if there are other usages of the isAJAX functionality.

Is there any other usage for isAJAX which we should add to this list?


Content negotiation as is seems pretty easy to use, maybe we should implement enums/constants with the "classic" web content types. Creating an alias isJSONin the request which internally uses content negotiation might make it even easier to use.