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(cloudflare-pages): handle assets only for get requests #968

Merged
merged 4 commits into from
Feb 21, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Feb 21, 2023

πŸ”— Linked issue

closes #796 - this is a remake as I had no permission to push changes to the branch (make sure to give credit when merging)
resolves #497
resolves #787

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

CF will throw a 405 error when fetching assets with a non-GET request. I think we can guard them here safely, with the note that we should also merge #965 alongside this to avoid hitting endpoint for known assets/asset prefixes.

πŸ“ Checklist

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

@danielroe danielroe added the bug Something isn't working label Feb 21, 2023
@danielroe danielroe requested a review from pi0 February 21, 2023 10:21
@danielroe danielroe self-assigned this Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #968 (1d4a591) into main (8f76db8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #968   +/-   ##
=======================================
  Coverage   67.34%   67.34%           
=======================================
  Files          60       60           
  Lines        6073     6073           
  Branches      680      680           
=======================================
  Hits         4090     4090           
  Misses       1974     1974           
  Partials        9        9           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

const asset = await ctx.next();
if (asset.status !== 404) {
return asset;
if (ctx.request.method === "GET") {
Copy link
Member

Choose a reason for hiding this comment

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

I think we might also support for HEAD

Suggested change
if (ctx.request.method === "GET") {
if (ctx.request.method === "GET" || ctx.request.method === "HEAD") {

Copy link
Member Author

@danielroe danielroe Feb 21, 2023

Choose a reason for hiding this comment

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

No, that is not allowed by Cloudflare either. You can confirm locally by building a nitro project and performing a HEAD request against it:

Before making this PR, I also tried with OPTIONS, as another promising one which might be useful for prerendered API routes, but unfortunately it's not allowed either.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm thanks for confirming.

This is strange because normal worker has fine gained API to control caching headers via head. (and i suppose would be useful for cors and options)

Copy link
Member Author

Choose a reason for hiding this comment

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

It would indeed be useful. Maybe they can add more options down the line to configuration via _routes.json?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's revise it later GET is good enough.

@pi0 pi0 changed the title fix: unable to handle non-GET request in cloudflare page fix(cloudflare-pages): handle assets only for get and head requests Feb 21, 2023
@danielroe danielroe changed the title fix(cloudflare-pages): handle assets only for get and head requests fix(cloudflare-pages): handle assets only for get requests Feb 21, 2023
@pi0 pi0 merged commit e47ffa0 into main Feb 21, 2023
@pi0 pi0 deleted the cloudflare-page-non-get-req branch February 21, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants