-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
allows head requests to return 200 instead of 403 #13100
Conversation
🦋 Changeset detectedLatest commit: 6e926f5 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a test and a changeset
CodSpeed Performance ReportMerging #13100 will not alter performanceComparing Summary
|
@@ -25,7 +25,7 @@ export function createOriginCheckMiddleware(): MiddlewareHandler { | |||
if (isPrerendered) { | |||
return next(); | |||
} | |||
if (request.method === 'GET') { | |||
if (request.method === 'GET' || request.method === "HEAD") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (request.method === 'GET' || request.method === "HEAD") { | |
if (request.method === 'GET' || request.method === 'HEAD' || request.method === 'OPTIONS' || request.method === 'TRACE') { |
There are 2 more safe request methods that should be added as exemptions besides GET and HEAD: OPTIONS and TRACE.
Maybe one should add centralized functions to define safe and unsafe functions/properties on request
? So one can do:
if (request.isSafe) { ... }
and reduce scope for making the same mistake in multiple places?
See source for definition of safe methods: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods#safe_idempotent_and_cacheable_request_methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is blocked because it contains a major
changeset. A reviewer will merge this at the next release if approved.
I believe may @corneliusroemer may have a better solution here #13101 |
Changes
fixes this issue #13079 by checking if the request method is "HEAD" it allows it to continue with the middleware and not return a 403
I don't believe the docs should need to be updated