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

Fixed Responses to OPTIONS Requests ignore Body #15108

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Kapsonfire-DE
Copy link
Contributor

Fixes #15082

Rules for containing body didnt align to Docs

…g/en-US/docs/Web/HTTP/Methods/CONNECT

Response to OPTIONS may contain response body
Request CONNECT must not contain body

Sorted removing alphabetically by METHOD NAME
@Kapsonfire-DE
Copy link
Contributor Author

Im not sure if these simple changes needs new tests.
things to consider in testings: do we have a server we can fetch with OPTIONS method and always returning a body? Issue Creator showed example with google, but whats about google changes to not providing a body?

src/http/method.zig Outdated Show resolved Hide resolved
Copy link
Member

@cirospaciari cirospaciari left a comment

Choose a reason for hiding this comment

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

We should follow the spec and add tests for it if anything is changed

https://datatracker.ietf.org/doc/html/rfc7231#section-4.3.1

   A payload within a GET request message has no defined semantics;
   sending a payload body on a GET request might cause some existing
   implementations to reject the request.

for CONNECT we need more work:

  CONNECT is intended only for use in requests to a proxy.  An origin
server that receives a CONNECT request for itself MAY respond with a
2xx (Successful) status code to indicate that a connection is
established.  However, most origin servers do not implement CONNECT.
 A server MUST NOT send any Transfer-Encoding or Content-Length header
fields in a 2xx (Successful) response to CONNECT.  A client MUST
ignore any Content-Length or Transfer-Encoding header fields received
in a successful response to CONNECT.

A payload within a CONNECT request message has no defined semantics;
sending a payload body on a CONNECT request might cause some existing
implementations to reject the request.

for OPTIONS we also need more checks:

  A client that generates an OPTIONS request containing a payload body
MUST send a valid Content-Type header field describing the
representation media type.  Although this specification does not
define any use for such a payload, future extensions to HTTP might
use the OPTIONS body to make more detailed queries about the target
resource.

@Kapsonfire-DE
Copy link
Contributor Author

shall I remove the .CONNECT Change for simpleness of this PR to fix the existing bug and address the OPTIONS request body later?

@cirospaciari
Copy link
Member

cirospaciari commented Nov 13, 2024

shall I remove the .CONNECT Change for simpleness of this PR to fix the existing bug and address the OPTIONS request body later?

You can reduce the context to only change the OPTIONS behavior but we will still need tests for Bun.serve, fetch and node:http and node:https

EDIT: I meant OPTIONS sorry

@Kapsonfire-DE
Copy link
Contributor Author

The current bug is, that Responses to OPTIONS have no body in bun, even though the server sends it. So I would reduce the changes, to allow OPTIONS Responses to have body and add a test for this.

So we can close this bug.

I'd address the OPTIONS Requests with Body in a later PR.
For CONNECT is more work to be done, so I'd remove it completely out of scope for this for now

@cirospaciari
Copy link
Member

The current bug is, that Responses to OPTIONS have no body in bun, even though the server sends it. So I would reduce the changes, to allow OPTIONS Responses to have body and add a test for this.

So we can close this bug.

I'd address the OPTIONS Requests with Body in a later PR. For CONNECT is more work to be done, so I'd remove it completely out of scope for this for now

Sure I meant OPTIONS sorry, you can reduce the context for only 1 change at a time sure, we still will need the tests.

@Kapsonfire-DE Kapsonfire-DE changed the title Fixed hasBody/hasRequestBody to align to https://developer.mozilla.or… Fixed Responses to OPTIONS Requests ignore Body Nov 14, 2024
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.

fetch with method: "OPTIONS" always returns ""
2 participants