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

Content-Type missing when returning new Response with 404 status #8627

Closed
1 task
wishrd opened this issue Sep 22, 2023 · 3 comments · Fixed by #9433
Closed
1 task

Content-Type missing when returning new Response with 404 status #8627

wishrd opened this issue Sep 22, 2023 · 3 comments · Fixed by #9433
Assignees
Labels
- P2: has workaround Bug, but has workaround (priority) feat: error pages Related to 404 and 500 handling (scope) requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs

Comments

@wishrd
Copy link

wishrd commented Sep 22, 2023

Astro Info

Astro                    v3.1.2
Node                     v18.15.0
System                   macOS (arm64)
Package Manager          npm
Output                   server
Adapter                  @astrojs/node
Integrations             none

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

No response

Describe the Bug

Declaring a dynamic route like /[param1]/[param2], if there is a return new Response(null, { status: 404 }) due to any reason in that route, the server side rendering of the 404.astro page happens but the Content-Type is missing.

I've the security header X-Content-Type-Options: nosniff which check the presence of the Content-Type, breaking the rendering of the 404 by the browser. Removing the header, the 404 page is rendered successfully.

Also, I've noticed the middleware I have adding the security header is executed twice in this case:

SECURITY MIDDLEWARE http://localhost:4321/es/asdasd HeadersList {
  cookies: null,
  [Symbol(headers map)]: Map(1) {
    'x-content-type-options' => { name: 'X-Content-Type-Options', value: 'nosniff' }
  },
  [Symbol(headers map sorted)]: null
}

SECURITY MIDDLEWARE http://localhost:4321/es/asdasd HeadersList {
  cookies: null,
  [Symbol(headers map)]: Map(2) {
    'content-type' => { name: 'content-type', value: 'text/html' },
    'x-content-type-options' => { name: 'X-Content-Type-Options', value: 'nosniff' }
  },
  [Symbol(headers map sorted)]: null
}

The first time, it doesn't contains the Content-Type but the second one yes. I think the browser is receiving the first one.

This is the use case we have in our application but probably it can be reproduced with other routes if a new Response is returned.

It has been tested building and running the server with a NodeJS adapter, all routes with SSR: npm run build && node dist/server/entry.mjs. Stackblitz is responding with error too for this use case but the response is a bit different:

  • Local: HTML rendered as string
  • Stackblitz: Error without rendering

I think the right fix is having the Content-Type and/or stop processing the request twice. If anyone prefer a Github repository instead of the stackblitz one, let me know. Thank you for your help!

What's the expected result?

The http response after returning a Response object by any route should have the Content-Type header.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-g7nwcp

  • Go to /whatever, it should display 404
  • Go to /test/test, it should display the page
  • Go to /test/whatever, it should display 404 but it doesn't

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 Sep 22, 2023
@ematipico
Copy link
Member

The reason why you see the middleware run twice is because the first time it runs for /[param1]/[param2], the second time runs for the custom 404.astro page.

@ematipico ematipico added - P4: important Violate documented behavior or significantly impacts performance (priority) feat: ssr Related to SSR (scope) - P2: has workaround Bug, but has workaround (priority) and removed needs triage Issue needs to be triaged feat: ssr Related to SSR (scope) - P4: important Violate documented behavior or significantly impacts performance (priority) labels Sep 22, 2023
@ematipico
Copy link
Member

Flagging it as P2, because it's possible to manually add the header to make it working

const { value } = Astro.params;

if (value !== 'test') {
    const res = new Response(null, { status: 404, });
    res.headers.append("content-type", "text/html")

    return res;
}

@wishrd
Copy link
Author

wishrd commented Sep 22, 2023

The reason why you see the middleware run twice is because the first time it runs for /[param1]/[param2], the second time runs for the custom 404.astro page.

I was guessing something like that could be the reason behind the double execution, yes. Thanks for the info and the triage! We will apply the workaround meanwhile.

@lilnasy lilnasy added the feat: error pages Related to 404 and 500 handling (scope) label Sep 22, 2023
@lilnasy lilnasy added the requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs label Dec 11, 2023
@ematipico ematipico self-assigned this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) feat: error pages Related to 404 and 500 handling (scope) requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants