fix: respond with 200 to HEAD requests for non-prerendered pages as well #13101
Conversation
🦋 Changeset detectedLatest commit: e4bf640 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
bot
added
the
pkg: astro
CodSpeed Performance ReportMerging #13101 will not alter performanceComparing Summary
|
|
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. |
|
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. |
| @@ -176,6 +176,34 @@ describe('CSRF origin check', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it("return a 200 when the origin doesn't match but calling HEAD", async () => { | |||
There was a problem hiding this comment.
Since this is quite a delicate feature that involves security, would you mind adding a test for each verb method that was added to SAFE_METHODS? I would like to cover all cases if possible
| request = new Request('http://example.com/api/', { | ||
| headers: { origin: 'http://loreum.com', 'content-type': 'multipart/form-data' }, | ||
| method: 'HEAD', | ||
| }); |
There was a problem hiding this comment.
The test is failing because you're hitting an endpoint, which exposes only the GET method.
You should:
- create an astro component and hit that route
- expand the
api.tsfile and add the rest of the verbs to test
There was a problem hiding this comment.
Thanks for the pointers!
| request.method === 'PATCH' || | ||
| request.method === 'DELETE') && | ||
| request.headers.get('origin') === url.origin; | ||
| const sameOrigin = request.headers.get('origin') === url.origin; |
There was a problem hiding this comment.
a nuance, imho isSameOrigin would fit better, as seen also in isPrerendered, hasContentType etc.
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