Ember client drop head body #7369

hamnis · 2024-01-26T20:31:51Z

Adds a flag to ember-core Parser.Response.parse to discard the body

this is only enabled for HTTP/1.1 HEAD requests, but there may be other http methods which does not expect a body in the response.

Not sure if this is the best location for this code, but the reproducer now passes.

Only enabled if the request is a HEAD request. This should only be an issue for HTTP/1.1 as http/2 body frames should not be generated for HEAD. Fixes http4s#7362

ember-core/shared/src/main/scala/org/http4s/ember/core/Parser.scala

hamnis · 2024-02-02T14:42:37Z

@danicheg @armanbilge please re-review this, @yanns confirmed that this fix works.

danicheg · 2024-02-02T15:10:07Z

It's ready to go, as far as I'm concerned. 👍🏻 But @armanbilge requested themselves to make a review. So the ball's in their court.

valencik

This seems reasonable.
At first I was confused why we have a discardBody and a expectNoBody, but expectNoBody only considers the Status, not the Method.

ember-core/shared/src/test/scala/org/http4s/ember/core/ParserSuite.scala

armanbilge · 2024-02-02T16:09:20Z

ember-core/shared/src/main/scala/org/http4s/ember/core/Parser.scala

@@ -373,6 +373,11 @@ private[ember] object Parser {
    def parser[F[_]: Concurrent](maxHeaderSize: Int)(
        buffer: Array[Byte],
        read: Read[F],
+    ): F[(Response[F], Drain[F])] = parser[F](maxHeaderSize, discardBody = false)(buffer, read)
+
+    def parser[F[_]: Concurrent](maxHeaderSize: Int, discardBody: Boolean)(


Is discardBody really the right name? AFAICT there is no discarding happening, it seems more similar to expectNoBody. Perhaps we can have requestExpectsNoBody and statusExpectsNoBody.

Sorry, I did some more reading.

Warning: A response to a HEAD method should not have a body. If it has one anyway, that body must be ignored: any representation headers that might describe the erroneous body are instead assumed to describe the response which a similar GET request would have received.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

So it does seem like the body must be discarded if it's present. So now I'm not sure if it's appropriate to share a codepath with statusExpectsNoBody.

armanbilge · 2024-02-02T17:28:50Z

ember-core/shared/src/test/scala/org/http4s/ember/core/ParserSuite.scala

+      assertEquals(body, "")
+      assertEquals(rest, "helloeverything after the body")


I'm pretty sure we should be asserting that both of these are empty. Otherwise we haven't really discarded the body, just left it on the socket.

are you sure about that? then expectNoBody would also leave data on the socket.

Right, but I don't think that "discard body" and "expect no body" are the same situation at all. See #7369 (comment).

For HEAD requests the spec mandates that we discard the body, if it's present. Meanwhile, the spec for e.g. 204 No Content makes no such mandate: it's up to us to decide how to handle that situation.

hamnis · 2024-02-16T11:10:46Z

@armanbilge can you please do another pass on this?

yanns · 2024-03-10T08:54:50Z

This issue is blocking migration from blaze to ember client. It'd be great to have the fix merged and released. Thanks all!

yanns · 2024-04-08T09:15:31Z

@armanbilge sorry for the ping, but would you have time to check this out?

rossabaker · 2024-05-02T03:59:02Z

I need to get a release out this week. I don't typically work on Ember, but given the positive feedback and that the interface changes are private, I'm inclined to merge this someone objects tomorrow.

valencik · 2024-05-02T01:54:01Z

ember-core/shared/src/test/scala/org/http4s/ember/core/ParserSuite.scala

@@ -364,6 +364,39 @@ class ParsingSuite extends Http4sSuite {
    }
  }

+  test(
+    "Parser.Response.parser return everything if the body is discarded"


Now that we've changed the behaviour to discard the rest perhaps we should update this test name?

Suggested change

"Parser.Response.parser return everything if the body is discarded"

"Parser.Response.parser returns empty body and rest when the body is discarded"

valencik · 2024-05-02T11:50:01Z

ember-core/shared/src/main/scala/org/http4s/ember/core/Parser.scala

@@ -406,7 +411,11 @@ private[ember] object Parser {
        )

        resp <-
-          if (headerP.chunked) {
+          if (discardBody) {
+            (baseResp -> none[Array[Byte]].pure[F]).pure[F]


Disclaimer, I haven't looked at the ember parser before.

What is this doing that is different from the line two below?

The return type is F[(Response[F], Drain[F])]. Presumably the thing on the right is what we are draining? Or is it the "drained bytes" 🤔 ?

My concern is whether or not

if (discardBody) { (baseResp -> none[Array[Byte]].pure[F]).pure[F]

is actually draining things from the socket or just saying "there wasn't anything on the socket".

rossabaker · 2024-05-02T16:50:58Z

ember-core/shared/src/test/scala/org/http4s/ember/core/ParserSuite.scala

@@ -364,6 +364,39 @@ class ParsingSuite extends Http4sSuite {
    }
  }

+  test(
+    "Parser.Response.parser return everything if the body is discarded"


What happens to the connection in this case? Discarding an ad hoc number of bytes and then continuing to parse makes me nervous about smuggling attacks.

This path is only for response parsing, right?

Is the expectation that the connection gets closed, even after a properly framed HEAD response?

rossabaker

As far as I can tell, Ember is closing the connection after a successful HEAD request, both on series/0.23 and on this branch. This doesn't make that problem worse and fixes the original problem. Furthermore, if it's too aggressively closing connections, it at least relieves us of worrying about the security implications of excess bytes in a response.

danicheg · 2024-05-03T16:50:34Z

Amazing work, team! Additional gratitude to @rossabaker for going an extra mile on verifying this 🙇

hamnis added module:ember-client module:ember-core labels Jan 26, 2024

mergify bot added the series/0.23 PRs targeting 0.23.x label Jan 26, 2024

hamnis linked an issue Jan 26, 2024 that may be closed by this pull request

Ember client hangs when parsing response body from HEAD request #7362

Closed

armanbilge self-requested a review January 26, 2024 20:34

hamnis force-pushed the ember-client-drop-head-body branch from 1d759b5 to 5545726 Compare January 26, 2024 20:45

mergify bot added the module:ember-server label Jan 26, 2024

hamnis force-pushed the ember-client-drop-head-body branch from 5545726 to a26d69e Compare January 26, 2024 21:04

Add flag to Parser.Response.parse to discard body

1c53199

Only enabled if the request is a HEAD request. This should only be an issue for HTTP/1.1 as http/2 body frames should not be generated for HEAD. Fixes http4s#7362

hamnis force-pushed the ember-client-drop-head-body branch from a26d69e to 1c53199 Compare January 26, 2024 21:44

danicheg reviewed Jan 27, 2024

View reviewed changes

ember-core/shared/src/main/scala/org/http4s/ember/core/Parser.scala Outdated Show resolved Hide resolved

use overload

bae1ad2

valencik reviewed Feb 2, 2024

View reviewed changes

ember-core/shared/src/test/scala/org/http4s/ember/core/ParserSuite.scala Outdated Show resolved Hide resolved

armanbilge reviewed Feb 2, 2024

View reviewed changes

restore test names

1bd393a

hamnis force-pushed the ember-client-drop-head-body branch from d2fdc1d to 1bd393a Compare February 2, 2024 17:24

armanbilge reviewed Feb 2, 2024

View reviewed changes

discardBody as own codepath

7e5a974

armanbilge self-requested a review February 21, 2024 22:29

kciesielski mentioned this pull request Feb 28, 2024

[BUG] HEAD request times out using Http4s backend softwaremill/sttp#2016

Closed

hamnis requested a review from a team April 30, 2024 15:23

yanns approved these changes Apr 30, 2024

View reviewed changes

valencik reviewed May 2, 2024

View reviewed changes

rossabaker reviewed May 2, 2024

View reviewed changes

rossabaker approved these changes May 3, 2024

View reviewed changes

rossabaker merged commit b42d37b into http4s:series/0.23 May 3, 2024
17 checks passed

hamnis deleted the ember-client-drop-head-body branch May 3, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ember client drop head body #7369

Ember client drop head body #7369

hamnis commented Jan 26, 2024

hamnis commented Feb 2, 2024

danicheg commented Feb 2, 2024

valencik left a comment

armanbilge Feb 2, 2024

armanbilge Feb 2, 2024

armanbilge Feb 2, 2024

hamnis Feb 2, 2024

armanbilge Feb 2, 2024

hamnis commented Feb 16, 2024

yanns commented Mar 10, 2024

yanns commented Apr 8, 2024

rossabaker commented May 2, 2024

valencik May 2, 2024

valencik May 2, 2024

rossabaker May 2, 2024

rossabaker May 3, 2024

rossabaker May 3, 2024

rossabaker left a comment

danicheg commented May 3, 2024

		assertEquals(body, "")
		assertEquals(rest, "helloeverything after the body")

	"Parser.Response.parser return everything if the body is discarded"
	"Parser.Response.parser returns empty body and rest when the body is discarded"

Ember client drop head body #7369

Ember client drop head body #7369

Conversation

hamnis commented Jan 26, 2024

hamnis commented Feb 2, 2024

danicheg commented Feb 2, 2024

valencik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamnis commented Feb 16, 2024

yanns commented Mar 10, 2024

yanns commented Apr 8, 2024

rossabaker commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

danicheg commented May 3, 2024