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

Inconsistency in Request constructor when rewriting a used body #674

Closed
harrishancock opened this issue Feb 26, 2018 · 11 comments

Comments

6 participants
@harrishancock
Copy link
Contributor

commented Feb 26, 2018

Script authors sometimes have a need to rewrite a Request's body, e.g. a service worker that transforms POST messages. There is currently no easy way to do this if the original body must first be read. Currently, the following code will throw:

let request = new Request("https://example.com/", {
  method: "POST",
  body: "an old body"
})

// Disturb the request's body.
let body = await request.text()

// Throws in Request ctor, step 7
let newRequest = new Request(request, {
  body: "a new body"
})

The last line throws, because Request's constructor is supposed to check that its first parameter is not disturbed (step 7) before it checks if its initialization dictionary contains a body property that will override the old body (steps 38-44).

This seems like a bug in the spec, because if we were to comment out the await request.text() line, the code snippet would work as intended: newRequest would have "a new body" as its body. That tells me that the intent was probably to allow authors to concisely rewrite bodies in just this way.

Note that even if step 7's check were modified or moved after the body extraction steps, there remains some ambiguity: if Request()'s first parameter is another Request object, step 43 unconditionally creates a new ReadableStream object from the old request's body, which I would expect to throw if the body were disturbed. Perhaps #463 will resolve that ambiguity.

/cc @kentonv

@annevk

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

It does seem reasonable to make this work, indeed.

cc @yutakahirano

@yutakahirano

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

We can do that, but I think #463 is not related.

@harrishancock

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2018

Great! In that case, I suppose one possible fix would be as simple as changing Request's constructor's step 7.1 from:

If input is disturbed or locked, then throw a TypeError.

to:

If input is disturbed or locked and init's body member is not present, then throw a TypeError.

If that's acceptable, I can prepare a PR. I guess there's not really any way to write a WPT test for this change, since I'm unaware of any browser implementation of request.body?

@yutakahirano

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

That is not enough as in the first comment. Changing step 43 to

inputBody is non-null and inputBody is body, ...

will solve the issue.

@annevk

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

@wanderview do you know if we have an implementation of request.body behind a flag in Firefox? Or if there are tests for that feature somewhere?

@wanderview

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

We don't have any request.body implementation yet.

harrishancock added a commit to harrishancock/fetch that referenced this issue Feb 28, 2018

Allow used body replacement in Request constructor
Currently, the following code snippet replaces `request`'s body as one
would expect:

  let request = new Request(url, { method: "POST", body: "foo" })
  request = new Request(request, { body: "bar" })

But this snippet throws a TypeError early in Request's constructor:

  let request = new Request(url, { method: "POST", body: "foo" })
  await request.text()  // disturb the body
  request = new Request(request, { body: "bar" })  // throws

This commit's changes allows the latter code snippet to work like the
first one.

Fixes whatwg#674.
@youennf

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Safari Tech Preview probably has request.body

@harrishancock

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@youennf good call. I don't have a Mac handy, but I downloaded Epiphany Tech Preview on Linux, which (I think?) uses the same WebKit that Safari does. It indeed has request.body. And indeed, it exhibits the throwy behavior I described in this issue.

In that case, I can explore writing a WPT test for this.

Also, I have a PR up for this at #675 -- I'm working on getting participant approval on my end.

@youennf

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2018

Nice to see Epiphany TechPreview in action :)

@mcatanzaro

This comment has been minimized.

Copy link

commented Mar 1, 2018

(Interrupting this fetch discussion for a tangent:

I don't have a Mac handy, but I downloaded Epiphany Tech Preview on Linux, which (I think?) uses the same WebKit that Safari does.

Ah! It's new, we don't have any telemetry, and you're the first person I've heard of to use it, besides me. That's cool. Congrats.

A bunch of code is platform-specific and WebKit on Linux has more bugs than macOS does, but yes, it's upstream WebKit, not a fork or anything. Currently it's based on a stable branch that branched off of WebKit trunk about three weeks ago, in preparation for a new stable release series. In about a month or so, we'll switch back to using snapshots of trunk.

And we just made a huge experimental UI change to Epiphany's address bar two days ago... it's not quite finished, so overlook that please. ;)

Sorry for the tangent!)

@harrishancock

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

Thanks for the info @mcatanzaro! It was in fact your blog post that led me to try it out. It's great to have one more implementation to test against on Linux.

harrishancock added a commit to harrishancock/fetch that referenced this issue Mar 5, 2018

Allow used body replacement in Request constructor
Currently, the following code snippet replaces `request`'s body as one
would expect:

  let request = new Request(url, { method: "POST", body: "foo" })
  request = new Request(request, { body: "bar" })

But this snippet throws a TypeError early in Request's constructor:

  let request = new Request(url, { method: "POST", body: "foo" })
  await request.text()  // disturb the body
  request = new Request(request, { body: "bar" })  // throws

This commit's changes allows the latter code snippet to work like the
first one.

Fixes whatwg#674.

@annevk annevk closed this in #675 Mar 17, 2018

annevk added a commit that referenced this issue Mar 17, 2018

Allow used body replacement in Request constructor
Currently, the following code snippet replaces request's body as one
would expect:

  let request = new Request(url, { method: "POST", body: "foo" })
  request = new Request(request, { body: "bar" })

But this snippet throws a TypeError early in Request's constructor:

  let request = new Request(url, { method: "POST", body: "foo" })
  await request.text()  // disturb the body
  request = new Request(request, { body: "bar" })  // throws

This commit's changes allows the latter code snippet to work like the
first one.

Tests: web-platform-tests/wpt#9931.

Fixes #674.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.