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: respond with 200 to HEAD requests for non-prerendered pages as well #13101

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

corneliusroemer
Copy link
Contributor

@corneliusroemer corneliusroemer commented Jan 30, 2025

Changes

Fixes #13079
Fixes #13104
Fixes #13103

Also ensure that HTTP method CONNECT gets origin checked as well (it was previously wrongly omitted because only POST, PUT, PATCH, DELETE were previously checked).

Inspired by @joshmkennedy's PR #13100

Testing

Add test for HEAD getting correct HTTP response 200 (this would have failed prior to his prior)

Docs

Pure bug fix, no docs changes necessary

Copy link

changeset-bot bot commented Jan 30, 2025

🦋 Changeset detected

Latest commit: 51b1adb

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 30, 2025
Copy link

codspeed-hq bot commented Jan 30, 2025

CodSpeed Performance Report

Merging #13101 will not alter performance

Comparing corneliusroemer:fix-csrf (51b1adb) with main (f392bef)

Summary

✅ 6 untouched benchmarks

@corneliusroemer
Copy link
Contributor Author

I don't understand why the test fails, is there maybe another big somewhere in HEAD handling? Or is this in the test utils?

When I test locally, I get 200 for head now.

@joshmkennedy
Copy link
Contributor

You will need to update the fixture. Here is what I did in my pull request. I just added a index.astro to the csrf-check-origin fixture. You could also add a API route for HEAD requests. Not sure which is best.

@ematipico
Copy link
Member

@corneliusroemer are still interested in this fix? If you don't have time, let us know, so we can carry over it to the finish line

@ematipico ematipico self-assigned this Feb 13, 2025
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I added more tests and removed TRACE because it isn't supported by undici

@ematipico ematipico merged commit 2ed67d5 into withastro:main Feb 13, 2025
16 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
4 participants