Cannot read request.formData |
||||||||||||
Issue descriptionChrome 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.
,
Dec 19
I'll take a look at this tomorrow.
,
Dec 20
,
Dec 20
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;
,
Dec 20
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.
,
Dec 20
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.
,
Dec 20
Out of curiosity, why did my reduced test fail to show the issue? Is it because the URLs are "same site"?
,
Dec 20
Hm I didn't investigate that yet, was just using the cross site example. That's a good question.
,
Dec 20
Don't worry about it. If you've got a reproduction of it, that's all that matters.
,
Dec 21
Jake: the reduced test case seems also repros this issue for me... strange.
,
Dec 21
seems to also repro...
,
Dec 21
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.
,
Dec 21
+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
,
Dec 21
+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
,
Dec 21
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.
,
Dec 25
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.
,
Dec 26
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!
,
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
,
Dec 31
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.
,
Dec 31
,
Dec 31
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
,
Jan 2
Approving merge to M2 branch 3626 based on comment #19. Please merge ASAP, Thank you.
,
Jan 2
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}
,
Jan 2
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
,
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
,
Jan 2
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}
,
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
,
Jan 7
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}
,
Jan 7
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 |
||||||||||||
Comment 1 by wanderview@chromium.org
, Dec 18