New issue
Advanced search Search tips

Issue 916070 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

Cannot read request.formData

Project Member Reported by jakearchibald@chromium.org, Dec 18

Issue description

Chrome Canary.

1. Visit https://share-target-bug--squoosh.netlify.com & wait for the service worker to install.
2. https://static-misc.glitch.me/file-post-test/ - Submit an image using the "live" section.

Console says:

serviceworker.js:132 Uncaught (in promise) TypeError: network error
Navigated to https://share-target-bug--squoosh.netlify.com/?share-target
serviceworker.js:132 Uncaught (in promise) TypeError: Failed to fetch
    at serveShareTarget (serviceworker.js:132)
    at serviceworker.js:310
serveShareTarget @ serviceworker.js:132
(anonymous) @ serviceworker.js:310
Promise.then (async)
serveShareTarget @ serviceworker.js:141
(anonymous) @ serviceworker.js:310
serviceworker.js:132 Uncaught (in promise) TypeError: Failed to fetch
    at serveShareTarget (serviceworker.js:132)
    at serviceworker.js:310

It appears to fail on calling request.formData().

The code is attempting to read the submitted image and open it in the app. It works as expected in Firefox Nightly.

I couldn't create a reduced case, but here's what I tried:

1. Visit https://static-misc.glitch.me/request-formdata/ & wait for the service worker to install.
2. https://static-misc-remix.glitch.me/request-formdata/ - Submit an image.

I've seen the above fail with the same error, but it mostly works fine.
 
The error only seems to happen with service worker servicification enabled for me.
Labels: -Pri-3 Target-72 Pri-1
Owner: falken@chromium.org
Status: Assigned (was: Untriaged)
I'll take a look at this tomorrow.
Status: Started (was: Assigned)
With yhirano I've narrowed this down to ComplexFormDataBytesConsumer failing here:

        case FormDataElement::kEncodedFile: {
          auto file_length = element.file_length_;
          if (file_length < 0) {
            if (!GetFileSize(element.filename_, file_length)) {
              form_data_ = nullptr;
              blob_bytes_consumer_ = BytesConsumer::CreateErrored(
                  Error("Cannot determine a file size"));
              return;

In non-S13nSW, we had a FileSizeResolver step in ServiceWorkerURLRequestJob before dispatching a fetch event to a SW.

In S13nSW there is no such step and the ResourceRequestBody goes straight to the SW. This body doesn't have the file size for uploaded files. A form upload creates a body using

#1 0x7fce8418f90f network::DataElement::SetToFilePathRange()
#2 0x7fce8419f3f4 network::ResourceRequestBody::AppendFileRange()
#3 0x7fce866ad477 content::GetRequestBodyForWebHTTPBody()
#4 0x7fce866accbe content::GetRequestBodyForWebURLRequest()
#5 0x7fce867fa9b1 content::RenderFrameImpl::BeginNavigationInternal()
#6 0x7fce867f8ecd content::RenderFrameImpl::BeginNavigation()

And this file has the default -1 length.

Probably ServiceWorkerNavigationLoader has to do a similar file size resolving step.

The fix is fairly straightforward but writing a test took most of my day.

After struggling with writing a test using beginDragWithFiles() I remembered tests need to be in http/local to work with beginDragWithFiles() and we have an existing test here but it's not testing formData() so it works with ServiceWorkerServicification. Furthmore it's disabled with Site Isolation at  bug 786510  but there's a non-SiteIsolation virtual test suite that still runs it. Need to work some more on this.
Out of curiosity, why did my reduced test fail to show the issue? Is it because the URLs are "same site"?
Hm I didn't investigate that yet, was just using the cross site example. That's a good question.
Don't worry about it. If you've got a reproduction of it, that's all that matters.
Jake: the reduced test case seems also repros this issue for me... strange.
seems to also repro...
I found that the existing test in http/tests/local/serviceworker/fetch-request-body-file.html can't repro the failure because for some reason Blink is able to call GetFileSize() to get size from the OS whereas it fails when running in a real browser. Probably the sandboxing is looser in http/local.

On to a browser test I guess.
Cc: lukasza@chromium.org creis@chromium.org
+lukasza, creis for Site Isolation or CORB expertise.

So I was wrong about the fix being simple. Even with file sizes resolved, Blink fails in formData().

To summarize, this bug happens when a page has a form with a <input type=file> element and does a POST to a cross-origin target. When that target has a service worker that intercepts the request and does event.request.formData() it throws an exception.

It looks like Blink is being denied access to the uploaded file. That caused GetFileSize() to fail at [1], which didn't happen for same-origin POSTs. In a WIP patch[2] I added a file resolving step in the browser process before sending the request to the service worker, which makes that call unnecessary, but now we fail later on when trying to read the body.

Now the failure is hard to trace through, but Blink is trying to read the uploaded form data via:

ComplexFormDataBytesConsumer reads from BlobBytesConsumer which reads from DataPipeAndDataBytesConsumer. DataPipeAndDataBytesConsumer tries to read from a data pipe using DataPipeGetter. DataPipeGetter is implemented by storage::BlobImpl which returns an error via DataPipeGetterReaderDelegate::OnComplete with network code -5, INVALID_HANDLE.

It looks like Site Isolation is somehow related. With my WIP patch that resolves file sizes, the bug doesn't occur in Chrome with --disable-site-isolation-trials. But the bug still occurs with the browser test I wrote, when running content_browsertests with that same flag. Does Site Isolation or anything else do something about files posted cross-origin?

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/fetch/form_data_bytes_consumer.cc?rcl=f6e87a44866c4860471163fc9fe217927bdb5a22&l=410
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1388132

Cc: mek@chromium.org
+mek in case you have hints how to debug this.

It looks like [1] iterates through the EncodedFormData and puts them in a BlobData. Then it makes BlobDataHandle::Create(std::move(blob_data), size) and BlobBytesConsumer tries to read from that. BlobBytesConsumer makes another EncodedFormData and gives it just a DataPipeGetter from  blob_data_handle_->AsDataPipeGetter(). Then makes a FormDataBytesConsumer (DataPipeAndDataBytesConsumer) that tries to read from that data pipe getter and fails.

I haven't been able to trace where something tries to read from the file but I assume that's what's failing.

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/fetch/form_data_bytes_consumer.cc?rcl=f6e87a44866c4860471163fc9fe217927bdb5a22&l=410

RE: Does Site Isolation or anything else do something about files posted cross-origin?

There is RenderFrameHostImpl::GrantFileAccessFromResourceRequestBody.  AFAIR this had to be introduced because XSSAuditor needs to read the POST-ed data to catch if the data is included in the html doc without proper escaping.  Maybe something similar needs to happen for service workers?

But... I am not sure if the above is the right lead, because I don't understand how granting access to a local file is related to *blobs* (which I thought are same-origin-only and can't be passed (e.g. via URL) to and used in a cross-origin frame).

PS. There might be other places where we had to grant file access for OOPIFs - it is probably easiest to find them by tracking down callers of ChildProcessSecurityPolicyImpl::GrantReadFile.
Thanks lukasza! It looks indeed like calling ChildProcessSecurityPolicyImpl::GrantReadFile is what fixes this. I should have a patch soon.

Regarding how blobs are related, I think blobs are just a collection of bytes, files, or other blobs, and it just happens to be how form data is packaged in some cases within Blink, so blob code ends up being used when doing POSTs.
Looks like in legacy code we used to package and send all the form data elements in a blob, that effectively also grants access to the elements via the blanket blob reference. In S13n code request body is transferred as is, it made the code need to do the explicit capability handling on its own. This was surely not very obvious outcome... nice detective work!
Project Member

Comment 18 by bugdroid1@chromium.org, Dec 26

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

commit c2148442b74470817a7895fed465d3f73d761e66
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Dec 26 09:38:38 2018

service worker: Grant the worker process access to files in the request body.

This fixes a bug when:
- A page has a form with a file input element.
- The form uploads to a cross-origin URL.
- A service worker intercepts the request and does Request.formData().

Request.formData() tries to access the elements in the request body.
But if an element was a File, the service worker's process usually won't
have permission to access to the file. This would cause the function to
throw an error.

The fix is to grant the process access to the files before dispatching
the fetch event. This bug only happened in S13nServiceWorker (or
NetworkService). In non-S13nServiceWorker the browser process builds a
new blob out of the request body and sends that blob to the service worker.
For blobs file access is checked when the file is added to the blob,
therefore sending the blob to a process virtually also transfers the
capability to the file to the process (so the process can access the file
but only via the blob reference).  On the other hand in Servicified code
path we started to transfer a request body as is, including raw file
paths, therefore we started to need this.

Bug:  916070 
Change-Id: Iab6eb430157dd880e123a6c5539582e734d527e6
Reviewed-on: https://chromium-review.googlesource.com/c/1388132
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618916}
[modify] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[add] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/test/BUILD.gn
[add] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/test/data/service_worker/file_upload_worker.js
[add] https://crrev.com/c2148442b74470817a7895fed465d3f73d761e66/content/test/data/service_worker/form.html

Labels: Merge-Request-72
Requesting merge to 72. This fixes a regression in 71 with ServiceWorkerServicification that was only discovered after SWS reached 100% on Stable. The fix is simple and has been on the Canary channel with no issue.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Project Member

Comment 21 by sheriffbot@chromium.org, Dec 31

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-72 Merge-Approved-72
Approving merge to M2 branch 3626 based on comment #19. Please merge ASAP, Thank you.
Labels: -Merge-Approved-72 Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09

Commit: e044a47d8a2f7bd2292bdb90d3f7e825bded9a09
Author: falken@chromium.org
Commiter: falken@chromium.org
Date: 2019-01-02 07:10:53 +0000 UTC

M72: service worker: Grant the worker process access to files in the request body.

This fixes a bug when:
- A page has a form with a file input element.
- The form uploads to a cross-origin URL.
- A service worker intercepts the request and does Request.formData().

Request.formData() tries to access the elements in the request body.
But if an element was a File, the service worker's process usually won't
have permission to access to the file. This would cause the function to
throw an error.

The fix is to grant the process access to the files before dispatching
the fetch event. This bug only happened in S13nServiceWorker (or
NetworkService). In non-S13nServiceWorker the browser process builds a
new blob out of the request body and sends that blob to the service worker.
For blobs file access is checked when the file is added to the blob,
therefore sending the blob to a process virtually also transfers the
capability to the file to the process (so the process can access the file
but only via the blob reference).  On the other hand in Servicified code
path we started to transfer a request body as is, including raw file
paths, therefore we started to need this.

Bug:  916070 
Change-Id: Iab6eb430157dd880e123a6c5539582e734d527e6
Reviewed-on: https://chromium-review.googlesource.com/c/1388132
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618916}(cherry picked from commit c2148442b74470817a7895fed465d3f73d761e66)
Reviewed-on: https://chromium-review.googlesource.com/c/1393131
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#534}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 2

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09

commit e044a47d8a2f7bd2292bdb90d3f7e825bded9a09
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jan 02 07:10:53 2019

M72: service worker: Grant the worker process access to files in the request body.

This fixes a bug when:
- A page has a form with a file input element.
- The form uploads to a cross-origin URL.
- A service worker intercepts the request and does Request.formData().

Request.formData() tries to access the elements in the request body.
But if an element was a File, the service worker's process usually won't
have permission to access to the file. This would cause the function to
throw an error.

The fix is to grant the process access to the files before dispatching
the fetch event. This bug only happened in S13nServiceWorker (or
NetworkService). In non-S13nServiceWorker the browser process builds a
new blob out of the request body and sends that blob to the service worker.
For blobs file access is checked when the file is added to the blob,
therefore sending the blob to a process virtually also transfers the
capability to the file to the process (so the process can access the file
but only via the blob reference).  On the other hand in Servicified code
path we started to transfer a request body as is, including raw file
paths, therefore we started to need this.

Bug:  916070 
Change-Id: Iab6eb430157dd880e123a6c5539582e734d527e6
Reviewed-on: https://chromium-review.googlesource.com/c/1388132
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618916}(cherry picked from commit c2148442b74470817a7895fed465d3f73d761e66)
Reviewed-on: https://chromium-review.googlesource.com/c/1393131
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#534}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[add] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/test/BUILD.gn
[add] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/test/data/service_worker/file_upload_worker.js
[add] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/test/data/service_worker/form.html

Project Member

Comment 25 by bugdroid1@chromium.org, Jan 2

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

commit cd4892df6760bbdd91b9ba65cef5c7241b629e21
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jan 02 08:56:17 2019

Revert "M72: service worker: Grant the worker process access to files in the request body."

This reverts commit e044a47d8a2f7bd2292bdb90d3f7e825bded9a09.

Reason for revert: Fails to compile.

Original change's description:
> M72: service worker: Grant the worker process access to files in the request body.
> 
> This fixes a bug when:
> - A page has a form with a file input element.
> - The form uploads to a cross-origin URL.
> - A service worker intercepts the request and does Request.formData().
> 
> Request.formData() tries to access the elements in the request body.
> But if an element was a File, the service worker's process usually won't
> have permission to access to the file. This would cause the function to
> throw an error.
> 
> The fix is to grant the process access to the files before dispatching
> the fetch event. This bug only happened in S13nServiceWorker (or
> NetworkService). In non-S13nServiceWorker the browser process builds a
> new blob out of the request body and sends that blob to the service worker.
> For blobs file access is checked when the file is added to the blob,
> therefore sending the blob to a process virtually also transfers the
> capability to the file to the process (so the process can access the file
> but only via the blob reference).  On the other hand in Servicified code
> path we started to transfer a request body as is, including raw file
> paths, therefore we started to need this.
> 
> Bug:  916070 
> Change-Id: Iab6eb430157dd880e123a6c5539582e734d527e6
> Reviewed-on: https://chromium-review.googlesource.com/c/1388132
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#618916}(cherry picked from commit c2148442b74470817a7895fed465d3f73d761e66)
> Reviewed-on: https://chromium-review.googlesource.com/c/1393131
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3626@{#534}
> Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

TBR=falken@chromium.org

Change-Id: I836dc15da0fbc6d231ed83e7a7126a9942d43777
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  916070 
Reviewed-on: https://chromium-review.googlesource.com/c/1393135
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#535}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/cd4892df6760bbdd91b9ba65cef5c7241b629e21/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[delete] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/cd4892df6760bbdd91b9ba65cef5c7241b629e21/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/cd4892df6760bbdd91b9ba65cef5c7241b629e21/content/test/BUILD.gn
[delete] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/test/data/service_worker/file_upload_worker.js
[delete] https://crrev.com/e044a47d8a2f7bd2292bdb90d3f7e825bded9a09/content/test/data/service_worker/form.html

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

Commit: cd4892df6760bbdd91b9ba65cef5c7241b629e21
Author: falken@chromium.org
Commiter: falken@chromium.org
Date: 2019-01-02 08:56:17 +0000 UTC

Revert "M72: service worker: Grant the worker process access to files in the request body."

This reverts commit e044a47d8a2f7bd2292bdb90d3f7e825bded9a09.

Reason for revert: Fails to compile.

Original change's description:
> M72: service worker: Grant the worker process access to files in the request body.
> 
> This fixes a bug when:
> - A page has a form with a file input element.
> - The form uploads to a cross-origin URL.
> - A service worker intercepts the request and does Request.formData().
> 
> Request.formData() tries to access the elements in the request body.
> But if an element was a File, the service worker's process usually won't
> have permission to access to the file. This would cause the function to
> throw an error.
> 
> The fix is to grant the process access to the files before dispatching
> the fetch event. This bug only happened in S13nServiceWorker (or
> NetworkService). In non-S13nServiceWorker the browser process builds a
> new blob out of the request body and sends that blob to the service worker.
> For blobs file access is checked when the file is added to the blob,
> therefore sending the blob to a process virtually also transfers the
> capability to the file to the process (so the process can access the file
> but only via the blob reference).  On the other hand in Servicified code
> path we started to transfer a request body as is, including raw file
> paths, therefore we started to need this.
> 
> Bug:  916070 
> Change-Id: Iab6eb430157dd880e123a6c5539582e734d527e6
> Reviewed-on: https://chromium-review.googlesource.com/c/1388132
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#618916}(cherry picked from commit c2148442b74470817a7895fed465d3f73d761e66)
> Reviewed-on: https://chromium-review.googlesource.com/c/1393131
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3626@{#534}
> Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

TBR=falken@chromium.org

Change-Id: I836dc15da0fbc6d231ed83e7a7126a9942d43777
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  916070 
Reviewed-on: https://chromium-review.googlesource.com/c/1393135
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#535}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 7

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

commit 7176d0939d74edf44ef8427a52427e239ec8e32c
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 07 03:43:50 2019

M72: service worker: Grant the worker process access to files in the request body.

This fixes a bug when:
- A page has a form with a file input element.
- The form uploads to a cross-origin URL.
- A service worker intercepts the request and does Request.formData().

Request.formData() tries to access the elements in the request body.
But if an element was a File, the service worker's process usually won't
have permission to access to the file. This would cause the function to
throw an error.

The fix is to grant the process access to the files before dispatching
the fetch event. This bug only happened in S13nServiceWorker (or
NetworkService). In non-S13nServiceWorker the browser process builds a
new blob out of the request body and sends that blob to the service worker.
For blobs file access is checked when the file is added to the blob,
therefore sending the blob to a process virtually also transfers the
capability to the file to the process (so the process can access the file
but only via the blob reference).  On the other hand in Servicified code
path we started to transfer a request body as is, including raw file
paths, therefore we started to need this.

Bug:  916070 
Change-Id: I4c4d5d808f1e0fb31e8a3988fdd5a86ba215ddd1
Reviewed-on: https://chromium-review.googlesource.com/c/1388132
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618916}
Reviewed-on: https://chromium-review.googlesource.com/c/1396014
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#577}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[add] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/browser/service_worker/service_worker_file_upload_browsertest.cc
[modify] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/test/BUILD.gn
[add] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/test/data/service_worker/file_upload_worker.js
[add] https://crrev.com/7176d0939d74edf44ef8427a52427e239ec8e32c/content/test/data/service_worker/form.html

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

Commit: 7176d0939d74edf44ef8427a52427e239ec8e32c
Author: falken@chromium.org
Commiter: falken@chromium.org
Date: 2019-01-07 03:43:50 +0000 UTC

M72: service worker: Grant the worker process access to files in the request body.

This fixes a bug when:
- A page has a form with a file input element.
- The form uploads to a cross-origin URL.
- A service worker intercepts the request and does Request.formData().

Request.formData() tries to access the elements in the request body.
But if an element was a File, the service worker's process usually won't
have permission to access to the file. This would cause the function to
throw an error.

The fix is to grant the process access to the files before dispatching
the fetch event. This bug only happened in S13nServiceWorker (or
NetworkService). In non-S13nServiceWorker the browser process builds a
new blob out of the request body and sends that blob to the service worker.
For blobs file access is checked when the file is added to the blob,
therefore sending the blob to a process virtually also transfers the
capability to the file to the process (so the process can access the file
but only via the blob reference).  On the other hand in Servicified code
path we started to transfer a request body as is, including raw file
paths, therefore we started to need this.

Bug:  916070 
Change-Id: I4c4d5d808f1e0fb31e8a3988fdd5a86ba215ddd1
Reviewed-on: https://chromium-review.googlesource.com/c/1388132
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618916}
Reviewed-on: https://chromium-review.googlesource.com/c/1396014
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#577}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Components: Internals>Services>Network
Labels: Proj-Servicification
Status: Fixed (was: Started)
Fixed in M72. NetworkService was also impacted by this, so adding those labels.

Looping back to some of the questions above, Site Isolation was probably involved because it makes the service worker start in a process other than the one that posted the form, resulting in lost access to the file. http/tests/local/serviceworker/fetch-request-body-file.html likely couldn't repro the failure because it only runs with SiteIsolation disabled due to  issue 786510 .

Sign in to add a comment