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

Rewriting doesn't use HTTP status code from rewritten response #11306

Closed
vchirikov opened this issue Jun 20, 2024 · 16 comments · Fixed by #11308, #11352 or #11477
Closed

Rewriting doesn't use HTTP status code from rewritten response #11306

vchirikov opened this issue Jun 20, 2024 · 16 comments · Fixed by #11308, #11352 or #11477
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)

Comments

@vchirikov
Copy link

vchirikov commented Jun 20, 2024

Astro Info

4.11.0

Describe the Bug

If we rewrite from non-existed page we always get 404 status even if middleware rewrite to the valid page.

Looks like the response object from rewriting isn't used. This is the repro:

bug

we should get http status 200 (because we rewrite to the /about page), but we see 404.

p.s. The issue was originally described here, this is separate issue for the bug with repro.
p.s. looks similar to the cookie issue (fixed by @ascorbic )

What's the expected result?

rewriting should use status code from rewritten page pipeline.

Link to Minimal Reproducible Example

https://github.com/vchirikov/astro_issue_02

@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jun 20, 2024
@ematipico ematipico added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged labels Jun 21, 2024
@ematipico ematipico self-assigned this Jun 21, 2024
@ematipico
Copy link
Member

@vchirikov do you experience the same issue with output: "server" (SSR)?

@vchirikov
Copy link
Author

Yes, btw the repro uses output: "server":

https://github.com/vchirikov/astro_issue_02/blob/f7c9532f3e31798f6acbbae748c076fe9fe185d9/astro.config.mjs#L7

@vchirikov
Copy link
Author

To be clear, the issue isn't about /404 rewriting, it's about wrong 404 status from rewritten page (/about in the gif), which should return 200, but returned 404 instead...

@ematipico
Copy link
Member

To be clear, the issue isn't about /404 rewriting, it's about wrong 404 status from rewritten page (/about in the gif), which should return 200, but returned 404 instead...

That wasn't clear from your "expectation" section. Can you be more clear, please? For example why /about should return 200 instead of 404? It's important to voice and justify expectations.

Also, could you reduce the branching of the middleware in your repro? It's not very clear where's the issue.

@vchirikov
Copy link
Author

could you reduce the branching of the middleware in your repro?

done in https://github.com/vchirikov/astro_issue_02/tree/issue-11306

Can you be more clear, please?

As a developer I expect that successfully rewritten page should return status 200 instead of 404:

image

@ematipico
Copy link
Member

As a developer I expect that successfully rewritten page should return status 200 instead of 404:

You haven't told me why though, because in this case you're intentionally rewriting with special route 404, which always returns a status code 404. Why should it return a 200? IMHO, it should not.

@vchirikov
Copy link
Author

vchirikov commented Jun 21, 2024

I rewrite all routes to next('/') (see middleware.ts on the screenshot), I don't get your you're intentionally rewriting with special route 404, which always returns a status code 404

I rewrite to an existing page, this page's response.status is 200 (!), I returned the Response object from middleware with status 200 (see the console.log in terminal + blue lines on the screenshot where I show Response with status code 200) but the Astro returns 404 to the browser.

Again rewriting correctly renders index.astro page if I use next('/') but with 404 status code despite the return code from index.astro which is 200 )


upd: oh, I think I get your point, I rewrite un-existed page and this is a special 404 route which always returns 404, but:

  1. it's unexpected behavior, I explicitly return the Response object with desired status code, but framework implicitly changes the response for me
  2. I can do this for another (existed) routes, it's inconsistency
  3. there is no docs about it

Example where it is useful:

I want to rewrite /${locale}/${url} to /${url} (and use /${url} as well, without copy-pasting the same pages to [..locale]) page and extract locale to the Astro.locals, with current behavior I will always get 404 on rewritten pages.

@vchirikov
Copy link
Author

vchirikov commented Jun 24, 2024

@ematipico the issue is not fixed in 4.11.1

image

repro: https://github.com/vchirikov/astro_issue_02/tree/issue-11306

npm run dev
# expected status is 200
curl -I -X GET http:https://localhost:4321/foo

@FugiTech
Copy link
Contributor

FugiTech commented Jul 7, 2024

@ematipico Apologies, but I don't believe this is fixed in 4.11.4 or 4.11.5. The repro above still returns 404 on both versions.

I also noted that the repro and your fix uses i18n, but personally I do not believe this issue is limited to using i18n. For example, my app is using rewriting to offer an archive of previous versions of the site under /v1/, /v2/ etc, while / is the latest version. It's very similar, but does not use or enable the i18n module. I'm happy to provide a repro without i18n if needed.

@ematipico
Copy link
Member

@FugiTech the fix I applied isn't related to i18n, however that's how the user found the bug and that's how I tested the fix.

Please open a new issue with a new reproduction and we will try to fix your issue too.

@vchirikov
Copy link
Author

vchirikov commented Jul 7, 2024

@ematipico the issue is still here in 4.11.5
image

p.s. repro the same - https://github.com/vchirikov/astro_issue_02

@vchirikov
Copy link
Author

@ematipico reopen please

@bgentry
Copy link

bgentry commented Jul 16, 2024

@ematipico There are definitely still issues with this. I don't have time at this exact moment to create a reproduction, but I have a super simple middleware that just intercepts some paths to return responses, otherwise passing the requests through:

export const onRequest: MiddlewareHandler = (context, next) => {
  const headers = {"Content-Type": "text/html"}; 
    return new Response(
      `<html>
        <head>
        </head>
        <body>
          <p>Some text</p>
        </body>
      </html>`,
      { headers, status: 200 },
    );
};

Despite the status: 200 in there, the response comes back with the provided body but with a 404 response status.

I will try to create a standalone reproduction this evening.

@ematipico ematipico reopened this Jul 17, 2024
@ematipico
Copy link
Member

I was on vacation. I will look into it.

@vchirikov
Copy link
Author

@ematipico I've updated the repro to 4.12.0, now the real http status is 200, but in stdout (logging) the status is 404.

expected response.status is 200
22:20:15 [404] /foo 4ms

@ematipico
Copy link
Member

Please open a new issue, the original one is fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)
Projects
None yet
5 participants