withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.8k stars 2.49k forks source link

Rewrite in middleware incorrectly returns a 404 response #11461

Closed FugiTech closed 3 months ago

FugiTech commented 4 months ago

Astro Info

Astro                    v4.11.5
Node                     v22.1.0
System                   Linux (x64)
Package Manager          bun
Output                   server
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/react
                         @astrojs/tailwind

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

  1. Create a new "just the basics" Astro project
  2. Enable experimental rewriting in the Astro config: experimental: { rewriting: true }
  3. Add a middleware.ts file that returns next(context.url.pathname === '/foo' ? '/' : undefined)
  4. Visit /: it will return a 200 with the body set to the index html
  5. Visit /foo: it will return a 404 with the body set to the index html
  6. Visit /bar: it will return a 404 with the body set to the default error page

I believe this is because /foo does not exist in src/pages, so Astro keeps the original 404 status code instead of respecting the status code of the rewrite. I believe it was previously reported as #11306 but the fix did not fully fix the issue.

This is especially a problem with Astro's prefetching, as the 404 prevents the prefetch from being used.

What's the expected result?

Visiting /foo should return a 200 status code

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-njawah?file=src%2Fmiddleware.ts

Participation

bgentry commented 4 months ago

I provided more details on the other issue here https://github.com/withastro/astro/issues/11306#issuecomment-2231254326, but this isn't just an issue with rewrites. A simple middleware that just returns a static 200 response on certain paths will also still return a 404 status despite what the Response says.

ematipico commented 4 months ago

Does this issue only affect the dev server, @FugiTech ?

ematipico commented 3 months ago

I provided more details on the other issue here https://github.com/withastro/astro/issues/11306#issuecomment-2231254326, but this isn't just an issue with rewrites. A simple middleware that just returns a static 200 response on certain paths will also still return a 404 status despite what the Response says.

That's unrelated to rewrites. Your middleware is triggered when rendering a 404. This was changed a few months ago, because users wanted to have their middleware triggered on 404 routes for some cleanup logic.

However, I'm not happy with this logic and I think we will change it (not sure how and when)

ematipico commented 3 months ago

Closing as fixed by https://github.com/withastro/astro/pull/11477

An upcoming release will fix the log: https://github.com/withastro/astro/pull/11505

bgentry commented 3 months ago

@ematipico interesting, so you're saying that the middleware which is allowed to return a Response is not actually expected to have its full response returned? That seems contrary to the concept of middleware which are typically called in a stack and have the opportunity to short circuit the rest of the stack, including the final endpoint.

ematipico commented 3 months ago

No, I am saying that if you hit a page that doesn't exist, the 404 route is rendered. While rendering the 404 route, the middleware is triggered, and you can do whatever you want. Returning a new Response might create unexpected situations (which I think is your case).

dahlia commented 3 months ago

I'm writing a middleware that effectively adds some more routes to the app, but those routes added by the middleware always respond with 404 Not Found, even though I returned a Response with 200 OK in the middleware. The response headers and the content body are maintained though. Is there any way to respond with other than 404 Not Found in this case?