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

Rewrite in middleware incorrectly returns a 404 response #11461

Closed
1 task
FugiTech opened this issue Jul 12, 2024 · 7 comments
Closed
1 task

Rewrite in middleware incorrectly returns a 404 response #11461

FugiTech opened this issue Jul 12, 2024 · 7 comments
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged

Comments

@FugiTech
Copy link
Contributor

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

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jul 12, 2024
@bgentry
Copy link

bgentry commented Jul 16, 2024

I provided more details on the other issue here #11306 (comment), 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
Copy link
Member

ematipico commented Jul 17, 2024

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

@ematipico ematipico added the needs response Issue needs response from OP label Jul 17, 2024
@ematipico
Copy link
Member

I provided more details on the other issue here #11306 (comment), 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
Copy link
Member

Closing as fixed by #11477

An upcoming release will fix the log: #11505

@bgentry
Copy link

bgentry commented Jul 19, 2024

@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
Copy link
Member

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
Copy link

dahlia commented Jul 21, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

4 participants