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

fix(error): respect accept: text/html request header #1921

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

passionate-bram
Copy link
Contributor

Requests made with the Fetch API cause sec-fetch-mode to be set, leading to these requests to be identified as JSON requests. The default error handler uses isJsonRequest to render errors. If a request were to set the Accept header to "text/html" the response should be HTML as well.

🔗 Linked issue

#1919

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

isJsonRequest is modified, its only usage (within nitro) is by the default event handler to determine when to serialize errors as HTML or JSON.

This change is needed because requests made with the Fetch API automatically get the sec-fetch-mode=cors header. This header is one of the conditions by which isJsonRequest function identifies JSON, but the header has no such meaning as far as the standard is concerned.

More importantly, the function does not provide any means to enforce an HTML response (for isJsonRequest to return false) when these fallback detections are used.

Resolves #1919

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

passionate-bram and others added 2 commits November 17, 2023 13:53
Requests made with the Fetch API cause `sec-fetch-mode` to be set, leading to these requests to be identified as JSON requests.
The default error handler uses isJsonRequest to render errors.
If a request were to set the Accept header to "text/html" the response should be HTML as well.
@pi0 pi0 changed the title fix: adhere to the accept http-header in the default error handler fix(error): respect accept: text/html request header Nov 17, 2023
In case the `Accept` header indicates both HTML and JSON, then HTML is preferred.
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pi0 pi0 merged commit 0a5911e into unjs:main Nov 20, 2023
4 checks passed
@pi0 pi0 mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The built-in error handler over-assumes json request type
2 participants