New issue
Advanced search Search tips

Issue 573937 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

GET request can incorrectly contain a body

Project Member Reported by jakearchibald@chromium.org, Jan 4 2016

Issue description

A service worker with:
self.addEventListener('fetch', event => event.respondWith(event.request));

Then, user submits a form that POSTs data, creating a manual-redirect POST request with a body. This is handled fine, but if the response is a 302, the resulting request has a method of GET, but the same body as the original POST, so it fails.

I don't have a reduced case, but it can be recreated on https://traintimes.org.uk/ - set a breakpoint at the fetch event, then search for a train from MAN to EUS. When the breakpoint hits, call event.respondWith(event.request) each time.

Error is "TypeError: Failed to execute 'fetch' on 'ServiceWorkerGlobalScope': Request with GET/HEAD method cannot have body."
 
The specs seem like they're missing bits when it comes to redirects, so also filed https://github.com/whatwg/html/issues/461 and https://github.com/whatwg/fetch/issues/187
Here's a more-reduced example https://adactio.com/swtest/success.php

Comment 3 by horo@chromium.org, Jan 5 2016

Owner: horo@chromium.org
Status: Assigned
That test link should be https://adactio.com/swtest/, sorry

Comment 5 by adr...@holovaty.com, Jan 18 2016

Running into this bug myself. Is there a workaround beyond manually excluding all possible redirect-after-POST URLs from my service worker?
In https://traintimes.org.uk/service-worker.js I add an extra `req = new Request(request.url)` step.

Comment 7 by adr...@holovaty.com, Jan 19 2016

Thanks -- at https://www.soundslice.com/serviceworker.js I ended up implementing the workaround like this:

request = new Request(request.url, {
    method: 'GET',
    headers: request.headers,
    mode: request.mode,
    credentials: request.credentials,
    redirect: request.redirect
});

The straight "new Request(request.url)" didn't work for me, as I needed the credentials passed through as well.

Note that I also tried "new Request(request, {body: ''})", but it didn't work. Hope this helps people in the future!
Labels: -Pri-2 Pri-1
I think we should consider things that break event.respondWith(fetch(event.request)) to be high priority, especially if they cannot be easily worked around by looking at the request.

Comment 9 by horo@chromium.org, Jan 22 2016

Status: Started
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f8aeea280e577fc8dddd2aeb6747f47f3ec502e

commit 8f8aeea280e577fc8dddd2aeb6747f47f3ec502e
Author: horo <horo@chromium.org>
Date: Mon Jan 25 07:35:36 2016

Don't send the body of the redirected request to the ServiceWorker.

The upload data in URLRequest is cleared while handing redirects in URLRequest::Redirect().
But ServiceWorkerURLRequestJob doesn't check it while re-sending the FetchEvent to the ServiceWorker.

This causes an error when "fetch(event.request)" is called in the ServiceWorker.
It is because request.body must not be set when request.method is GET.
To fix this problem, this patch adds has_upload() check in ServiceWorkerURLRequestJob::CreateFetchRequest().

BUG= 573937 
TEST=http/tests/serviceworker/navigation-redirect-body.html

Review URL: https://codereview.chromium.org/1612423002

Cr-Commit-Position: refs/heads/master@{#371196}

[modify] http://crrev.com/8f8aeea280e577fc8dddd2aeb6747f47f3ec502e/content/browser/service_worker/service_worker_url_request_job.cc
[add] http://crrev.com/8f8aeea280e577fc8dddd2aeb6747f47f3ec502e/third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-redirect-body.html
[add] http://crrev.com/8f8aeea280e577fc8dddd2aeb6747f47f3ec502e/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigation-redirect-body-worker.js
[add] http://crrev.com/8f8aeea280e577fc8dddd2aeb6747f47f3ec502e/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigation-redirect-body.php

Comment 11 by horo@chromium.org, Jan 26 2016

Status: Fixed
Labels: Needs-Feedback
horo@Could you please provide sample test file with reproducible steps to verify the fix from test team end.
Labels: M-50

Comment 14 by horo@chromium.org, Feb 1 2016

#10 has a layout test.

Execute "./third_party/WebKit/Tools/Scripts/run-blink-httpd" and go
http://127.0.0.1:8000/serviceworker/navigation-redirect-body.html

I just upgraded to Chrome 49.0.2623.87 (64-bit), and the workaround I did in Comment 7 is now breaking the service worker. Here's the error:

"Uncaught TypeError: Failed to construct 'Request': Cannot construct a Request with a RequestInit whose mode member is set as 'navigate'."

Comment 16 by bke...@mozilla.com, Mar 18 2016

See the discussion in here:

  https://github.com/whatwg/fetch/issues/245
As a workaround, you can recreate the request object without the body https://gist.github.com/jakearchibald/aff93cad208bd56a02ea70f9f0d01c99

Sign in to add a comment