getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.27k stars 167 forks source link

Conceptual oversight in 3.6? Redirects and Fiber #4084

Closed pReya closed 2 years ago

pReya commented 2 years ago

Description

I think I have found a rather big conceptual oversight in Kirby 3.6. With the Fiber architecture, most requests in the Panel are made via AJAX. However this will create unexpected behavior (the resulting behavior is described in #4054) , when the Kirby Backend returns a HTTP Redirect (Responses with a 302 code and a Location header). Even though an AJAX request will by default follow the HTTP redirect, the page which is the target of the redirect, will not be displayed (in contrast to a "proper" browser request, where the browser will automatically display the target of a redirect). So the result of these requests will just be shown in the little error modal with a JSON parsing error (because the response is a full HTML page, not just JSON).

I thought this would be easy to fix. Just check in JS if the response is a 302/redirect, and then manually navigate to the target given in the Location: header. However you can't access the Location: header via JS in an AJAX request (it's a security feature).

So I think the only way to fix this, will be to include some Location/Target property in the body of redirect responses, which can then be accessed in JS and manually redirected to.

Would love to have somebody from the Kirby team confirm this problem/oversight and/or mention some other solutions for this problem.

I thought I had a fix for this problem with #4077, but it's really not a proper solution.

bastianallgeier commented 2 years ago

I ran into the same problem and I agree that we need a fix for that. But I think this is solvable. I wanted to look into it as soon as 3.6.2 is done. Would be nice to already discuss ideas here though. Sending a JSON redirect object could be indeed a solution.

pReya commented 2 years ago

Thanks for the answer and confirming my theory. I agree, that this is totally solvable, but it's not just a bug (what I thought so far), but rather something that still needs a concept/plan.

I've just looked at Inertia.js and how they handle it. Seems they send a 409 response and use a custom header X-Inertia-Location, which would be another possible solution.

distantnative commented 2 years ago

I would agree that a custom header could be very fitting here.

pReya commented 2 years ago

The potential new response would probably also have to use another status code than 302. For a 302, fetch() will always follow the redirect, which we don't need, since we're going to request the proper location later anyway via a window.location.replace() or so. This is probably why Inertia.js chose 409. 302 would result in unnecessary requests/traffic.

pReya commented 2 years ago

I ran into the same problem and I agree that we need a fix for that. But I think this is solvable. I wanted to look into it as soon as 3.6.2 is done. Would be nice to already discuss ideas here though. Sending a JSON redirect object could be indeed a solution.

I know you hate talking about timeline, but I have a business decision to make, based on this feature. I tried working on this myself, but I'm not getting anywhere. Any idea when 3.6.2 will be ready/when you will tackle this?

rasteiner commented 2 years ago

I'm not getting anywhere.

What is your solution in the PR missing? I've done something similar and it seems to work. It's a hackier solution, though, as it's packaged as a plugin (so that it works independently of the kirby release cycle). Like yours it works with Response.url, but without redirect: 'manual' (Response.redirected instead). Maybe it works for you too, until a Proper Solution™ comes around.

plugin index.js

(function() {
  const f = fetch;

  window.fetch = async function(url, options) {
    if(options?.headers?.['X-Fiber-Only'] !== undefined) {
      const res = await f(url, options);
      if(!res.headers.get('x-fiber') && res.redirected) {
        window.location.replace(res.url);
        return new Promise(() => {});
      }
      return res;
    } else {
      return f(url, options);
    }
  }
})();
bastianallgeier commented 2 years ago

@rasteiner's solution is pretty close. I've created a PR: https://github.com/getkirby/kirby/pull/4085

bastianallgeier commented 2 years ago