1376155 - webRequest: support modifying request bodies (e.g. via requestBody BlockingResponse)

Status: NEW

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Rob Wu [:robwu]]

Requirements for the extension API to modify request bodies:
- It should have a minimal impact on network performance.
- It should be able to handle arbitrarily large request bodies by design.
- It should be reasonably simple to use.

External references:
- Prominent use case: Tamper Data (https://addons.mozilla.org/firefox/addon/tamper-data/)
- Chromium bug: https://crbug.com/91191

A prerequisite for modifying request bodies is read access to the full request body. There is a "requestBody" property (from bug 1201979), but its current format is not suitable, because:
1) It does not offer access to the data of file uploads, or even a filename to identify the file.
2) There is no way to reverse the parsed "formdata" to its original value (order of form keys are changed, which affect servers that depend on a specific order of form elements).
3) The "raw" array can be truncated if the body is too large: https://searchfox.org/mozilla-central/rev/3345af4b5/toolkit/modules/addons/WebRequestUpload.jsm#258-269

1 could be fixed by adding a handle to the upload object (either file name or Blob instance).
2 cannot be fixed because all values are already grouped by key.
3 is a flaw in the API design: the "raw" array being an ArrayBuffer forces the body to be fully in-memory.

These flaws are difficult to fix because they are design errors, so let's try a new API to represent the request body (maybe using a new extraInfoSpec flag?).
For raw requests, use Blob instances. This allows the browser to use the filesystem to store the form data (instead of memory).

For form data, use FormData (DOM API).
Benefits of using FormData as an API:
- order of key-value pairs is preserved.
- Blob (and File) instances are supported.
- large values can be represented by Blob instances instead of (large) strings, if desired.

Implementation point of view:
- FormData is structurally cloneable (good for IPC).
- The spec of FetchEvent in service workers includes a Request instance, which inherits the formData method in its interface to retrieve a FormData for a request. So browsers that support Service Workers should already have the internals in place to obtain a FormData for a request.
In Firefox, dom/base/BodyUtil.cpp seems to contain form parsing logic.



Once the above improvement to reading request bodies is implemented, actual modification can simply be implemented by changing the uploadStream.setData at http-on-modify-request.


Here is an example of how the API can be used by WebExtension developers.

browser.webRequest.onBeforeRequest.addListener(function(details) {
  var formData = details.requestBody.newFormData;
  var chunk = fd.get('somekey');
  // TODO: Check whether chunk is a string or Blob/File.
  // Let's assume that it's a Blob instance, to demonstrate async stuff.
  return new Promise(function(resolve, reject) {
    var fr = new FileReader();
    fr.onload = function() {
      var formElementValue = fr.result; // ArrayBuffer
      // TODO: Modify form element via typed arrays.
      // Assuming that the value has been changed, update the form:
      formData.set('somekey', formElementValue);

      // And finish the onBeforeRequest event listener:
      resolve({
        requestBody: formData,
        // requestBody type = FormData | Blob | ArrayBuffer
      });
    };
    fr.onerror = reject;
    fr.readAsArrayBuffer(chunk);
  });
}, {
  urls: [ 'https://example.com/*' ],
}, ['blocking', 'requestBody']);

Comment 1

[Rob Wu [:robwu]]

Giorgio, since you wrote the initial implementation of requestBody (and rely on it in NoScript), can you review my proposed API change?

Comment 2

[Giorgio Maone [:ma1]]

It seems good to me, provided that we keep it as a new API (for cross-browser compatibility) and we provide clients with an easy way to discover and opt-in so there's no performance impact from double parsing.
As you said the ones you outlined are design flaws, mostly inherited from Chromium (except for the missing filename which is an implementation omission), so it would also make sense to carefully spec this and propose it for standardization.

Comment 3

[Rob Wu [:robwu]]

Shane, could you review this API proposal?

At some point I also want to sollicit feedback from fetch/network people (I have yet to figure out who those are), but first I want to see if the API from the extension point of view looks fine.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Lets get a new bug to fix filename per comment 2.


Is this functionality a blocker for something important?

I'm not crazy about "newFormData".  A cursory read of the chrome bug, I kind of like adding an option to extraInfoSpec that changes the behavior.  Perhaps something like requestBody.2

browser.webRequest.onBeforeRequest.addListener(function(details) {

    //requestBody.raw no longer exists
    //requestBody.stream replaces raw
    //requestBody.formData is the "newFormData" proposed

}, ['blocking', 'requestBody.2']);

I'd like to see an outline (similar to this bug) for how we will handle uploadStream.setData before the direction here is final.

What I'm not sure about is what the api looks like for writing a large requestBody, does formData handle any potential capability for that (e.g. raw binary data)?  For example, if one wanted to transform a binary stream (similar to transcoding video during download) would we be able to do something like:

browser.webRequest.onBeforeRequest.addListener(function(details) {

  let oldStream = details.requestBody.stream;
  details.requestBody.stream = newStreamObject;
  // addon can now have some kind of event pump to write the upload data
  return { requestBody };

}, ['blocking', 'requestBody.2']);

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

>     //requestBody.stream replaces raw

This could be a streamfilter instead, per bug 1255894

Comment 6

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Lets get a new bug to fix filename per comment 2.

Nobody complained about the missing file name so far, and just knowing the file name does not seem to be that useful (there is no file I/O). If we focus on fixing this improved API, I think that it's OK to not spend much more time on this.


> Is this functionality a blocker for something important?

Yes. This feature is critical for the Tamper Data add-on.
It allows users to modify the request headers and request body in the same screen.

> I'm not crazy about "newFormData".

This is of course not the final name. I picked this name to make it obvious that this is the new property that I'm proposing. If we do use a new extraInfoSpec, then ".formData" sounds good to me.

> I'd like to see an outline (similar to this bug) for how we will handle
> uploadStream.setData before the direction here is final.

What do you mean by this?

> What I'm not sure about is what the api looks like for writing a large
> requestBody, does formData handle any potential capability for that (e.g.
> raw binary data)?

FormData is meant for forms, and the API supports Blob/File as values (not just strings).
For unformatted "raw" data, I proposed Blobs, which are basically handles to fixed-size data and don't require much memory.

A writable stream implies that the data size is variable, and that only makes sense for requests with Transfer-Encoding:chunked. Servers may not support chunked requests, so we should refrain from offering a way to switch from fixed-size (blob) request bodies to variable-size streams. Or shouldn't we? (we can also simply document the risk and allow extensions to make this choice).

In case we do want to support streams, I think that a stream filter similar to bug 1255894 can make sense. What if multiple extensions try to use it though?

And also, ideally an extension should see whether it has or has not succeeded in overwriting the request body (e.g. as a parameter to the webRequest.onBeforeSendHeaders event).

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #6)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > Lets get a new bug to fix filename per comment 2.
> 
> I think that it's OK to not spend much
> more time on this.

only asking for a bug so it is tracked.

> > I'm not crazy about "newFormData".
> 
> This is of course not the final name. I picked this name to make it obvious
> that this is the new property that I'm proposing. If we do use a new
> extraInfoSpec, then ".formData" sounds good to me.

I'd rather version it with a number, but not a strong opinion.

> > I'd like to see an outline (similar to this bug) for how we will handle
> > uploadStream.setData before the direction here is final.
> 
> What do you mean by this?

You said: "Once the above improvement to reading request bodies is implemented, actual modification can simply be implemented by changing the uploadStream.setData at http-on-modify-request."

Is that a followup bug, or a part of this work?  It sounds like a followup.  Is write functionality something we expose (e.g. replacing raw with stream)?

> > What I'm not sure about is what the api looks like for writing a large
> > requestBody, does formData handle any potential capability for that (e.g.
> > raw binary data)?


...

I'm asking because the api you describe only implements formData.  This is why I added the suggestion for a stream.  Otherwise, this bug is only about formData.  That is fine, but I want to be clear what is being tackled or not.


> And also, ideally an extension should see whether it has or has not
> succeeded in overwriting the request body (e.g. as a parameter to the
> webRequest.onBeforeSendHeaders event).

I'd prefer to throw some kind of error if it didn't succeed.  Otherwise, who's flag is it if a chain of extensions modify the formData?

Comment 8

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> (In reply to Rob Wu [:robwu] from comment #6)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > > Lets get a new bug to fix filename per comment 2.
> > 
> > I think that it's OK to not spend much
> > more time on this.
> 
> only asking for a bug so it is tracked.

Bug 1268885 has been filed before.

> > > I'd like to see an outline (similar to this bug) for how we will handle
> > > uploadStream.setData before the direction here is final.
> > 
> > What do you mean by this?
> 
> You said: "Once the above improvement to reading request bodies is
> implemented, actual modification can simply be implemented by changing the
> uploadStream.setData at http-on-modify-request."
> 
> Is that a followup bug, or a part of this work?  It sounds like a followup. 
> Is write functionality something we expose (e.g. replacing raw with stream)?

This is part of the work. First I introduce an accurate representation of the request body, then I implement replacing the existing upload stream. Even if internally the request body is implemented as a stream, the API does not need to provide the ability to intercept individual chunks of a stream.

> I'm asking because the api you describe only implements formData.  This is
> why I added the suggestion for a stream.  Otherwise, this bug is only about
> formData.  That is fine, but I want to be clear what is being tackled or not.

Fixed-size request bodies are my primary focus (FormData / Blob).
Stream support is outside of the scope of this bug, because I haven't found a concrete use case for it. When I encounter such a stream, I will possibly just wait until the last chunk has been sent, and then pass the whole Blob to the webRequest API consumer.

I will also evaluate the possibility of lazily generating the request body, via callbacks or extra webRequest methods. It would be unfortunate if an extension listens on all requests with requestBody.2 if it only need it for a fraction of the requests.

> > And also, ideally an extension should see whether it has or has not
> > succeeded in overwriting the request body (e.g. as a parameter to the
> > webRequest.onBeforeSendHeaders event).
> 
> I'd prefer to throw some kind of error if it didn't succeed.
> Otherwise, who's flag is it if a chain of extensions modify the formData?

If the response body is set through a return value only, then there is no way to signal errors in the API as exists today.
If this flag were to be a boolean, then it would be true iff the request body of the extension was accepted.

Comment 10

[ethan.devacc]

Hello!
I created an account just for this ticket. Can we consider adding this feature again? :))