feat: net.fetch() supports custom protocols

[nornagon]

Description of Change

Allow net.fetch() to request protocols other than http: and https:,
including file: and custom protocols.

Notably this doesn't support chrome-extension: or chrome: URLs, nor does it
allow differentiating between various different modes of URL loading
(navigation, subresource, worker main resource, service worker update) which
Chromium does differentiate internally.

This is also duplicating logic that exists elsewhere and we likely need to
clean this up. There are several considerations that need to be made when
requesting a URL in Electron:

• Is there a webRequest handler that needs to be invoked?
• Has any extension registered a chrome.webRequest handler? (This is not the
  same thing as webRequest.)
• Is this a request to a custom protocol (registered with
  protocol.register*Protocol)?
• Is there an intercept handler registered for this protocol (from
  protocol.interceptProtocol)?
• Should the connection limit be applied to this request? (i.e. is the domain listed in --ignore-connections-limit)?
• Should access to file: be allowed? (We do some hacks here to allow service workers to be loaded in file:// domains; arguably we should not do this)

The logic for handling these are spread around the code.

• ProxyingURLLoaderFactory handles the webRequest API and intercepted protocols, as well as ignoring connection limits for certain requests.
• Custom protocol handlers are installed in the RegisterNonNetwork* methods of ElectronBrowserClient. These methods also make decisions about where to allow file: access and where not to, based on the specific method.
• Custom protocol handlers are manually checked in InspectableWebContents::LoadNetworkResource to support devtools. NB that code doesn't currently use ProxyingURLLoaderFactory so it doesn't support webRequest or intercepted protocols.
• We carve out an exception for service workers in file: urls inside ProxyingURLLoaderFactory.

Ideally I think it should be possible to use net.fetch to produce a
reasonably good facsimile of any request that a renderer could generate. This
gets us a little bit closer to that ideal.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation, tutorials, templates and examples are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Changed net.fetch to support requests to file: URLs and custom protocols registered with protocol.register*Protocol.

[dsanders11]

Let's make sure file: can be intercepted since it's a special case compared to other schemes.

[MarshallOfSound]

Let's make sure file: can be intercepted since it's a special case compared to other schemes.

I would argue for safety and to avoid a common security footgun to simply prevent usage of file://* by default, heck I'd even consider making custom protocols an opt-in thing in the net.request options or something if possible. If folks are just passing URLs into net.request without allowlisting them this suddenly elevates to RFR 🤷

TLDR: This seems dangerous, can we make it safe by default?

[nornagon]

Let's make sure file: can be intercepted since it's a special case compared to other schemes.

I would argue for safety and to avoid a common security footgun to simply prevent usage of file://* by default, heck I'd even consider making custom protocols an opt-in thing in the net.request options or something if possible. If folks are just passing URLs into net.request without allowlisting them this suddenly elevates to RFR 🤷

TLDR: This seems dangerous, can we make it safe by default?

Can you tell me more about the threat model you're imagining? I feel like putting arbitrary user-controlled URLs into net.request without validation and transmitting the result off-machine is already a security issue. Allowing file: and custom protocols expands the surface area somewhat but I'm not sure putting it behind some sort of "I know what I'm doing" flag would do anything real to improve security. Apps should validate the URLs they feed to net.request.

[MarshallOfSound]

Can you tell me more about the threat model you're imagining? I feel like putting arbitrary user-controlled URLs into net.request without validation and transmitting the result off-machine is already a security issue.

I'm not sure specifically, but I don't think fetching http/s user controlled URLs and doing something with the result is inherently unsafe. But allowing those URLs to transparently be file:// URLs is definitely unsafe in that scenario.

[nornagon]

Can you tell me more about the threat model you're imagining? I feel like putting arbitrary user-controlled URLs into net.request without validation and transmitting the result off-machine is already a security issue.

I'm not sure specifically, but I don't think fetching http/s user controlled URLs and doing something with the result is inherently unsafe. But allowing those URLs to transparently be file:// URLs is definitely unsafe in that scenario.

I think it really depends on what "doing something with the result" is. For example, even if the response data is completely ignored, if an attacker controls an http request completely, that allows the attacker to:

1. Exfiltrate the user's IP address
2. Collect any other information that is intentionally or otherwise sent along with the request, for instance, app/OS version or locale, as well as any information that happens to be attached to the request headers by the app
3. DOS the user's internet connection by requesting an unending data stream
4. Potentially exploit any network stack bugs, especially a concern if the application is outdated

I'd say that these are already enough to warrant careful attention to controlling the URLs that are fed to net.request. These are pretty much the same as the problems you might get if you allowed an attacker to control the src attribute of an <img>, which is something that apps guard against with Content-Security-Policy and other measures. So it doesn't seem terribly unreasonable that apps might apply the same sort of caution here that they would apply to feeding tainted URLs to <img src>.

---

From a different angle, let me give a little background about why I want to make this change. I am planning a refactor of the protocol module in Electron, and as part of that refactor I want to allow apps to use net.request to "forward" a request on to other protocols. This would replace the current functionality of {register,intercept}{File,Http}Protocol. For example,

protocol.handle('https', async (req) => {
  if (req.url === 'https://foo.com/replaced') {
    return '<body>this is some replacement content</body>'
  } else {
    // Need to specify skipCustomHandlers to avoid recursing.
    const r = net.request({...req, skipCustomHandlers: true}).end()
    const response = await new Promise(resolve => req.on('response', resolve))
    response.headers['New-Header'] = 'New-Header-Value'
    return response
  }
})

This is more flexible and more powerful than the existing APIs, in that:

1. It allows apps to call into the "default" handling of the protocol by the network service or other handlers.
2. It allows apps to manipulate the response headers of the "upstream" handler before forwarding them back to the browser, similar to how webRequest works.

Ideally the underlying system would be clever enough to avoid going through JS for each data chunk, and this would have roughly similar performance to an unintercepted request that was manipulated by webRequest.

So that's the motivation. (Now that I write this out, I'm also seeing that we'll need to make sure that redirects to other schemes are rejected by default, as they are in the browser.) Perhaps you can think of other ways to achieve these goals?

[MarshallOfSound]

Perhaps you can think of other ways to achieve these goals?

Just at a glance your proposed API isn't super easy to use, especially around error handling, processing of request bodies, etc.

I don't know how technically complex this API design would be but I think the nicest way of handling your problem space from an API perspective would be this. (Some objects filled for completeness but some properties should be optional)

protocol.handle('https', async (req) => {
  if (req.url === 'https://foo.com/replaced') {
    return {
      headers: {}, // optional
      statusCode: 200, // optional
      body: '<body>this is some replacement content</body>'
    };
  } else {
    const response = await req.passthrough();
    response.headers['New-Header'] = 'New-Header-Value'
    return response
  }
})

protocol.handle('my-app', async (req) => {
  const response = await req.rewrite({
    headers: {}, // optional
    body: 'new-body', // optional
    url: 'file:///etc/foo.conf,
  });
  response.headers['New-Header'] = 'New-Header-Value'
  return response
})

Whether this is implemented by custom magic flags to the net module and handling errors appropriately or not is an implementation detail, but I don't believe it makes sense to expose the file: or custom-protocol: primitives to the net module given the usecase you are solving for doesn't actually necessitate it.

[PraveenNanda124]

Good work

[nornagon]

@MarshallOfSound hm, I agree the net.request API is a little clunky here, but the rewrite / passthrough API also seems kind of odd to me, and doesn't have the advantage of being able to execute requests "like a WebContents" outside of the context of a request made from a WebContents.

What about introducing a net.fetch() API which behaves similarly to Node/Web fetch? I'm thinking it would be nice to sort of follow roughly how Service Workers handle requests, which is like:

self.addEventListener("fetch", (event) => {
  event.respondWith(
    (async () => {
      const cachedResponse = await cache.match(event.request);
      if (cachedResponse)
        return cachedResponse;

      return fetch(event.request);
    })()
  );
});

I don't think we need the respondWith wrapper, which is mostly sugar for event.preventDefault(), but notice that the handler just returns the fetch promise to forward it on to the network stack.

We could do something similar:

protocol.handle('https', (request) => {
  if (request.url === 'https://thing')
    return '<body>custom</body>'
  return net.fetch(request)
})

or, to forward to a different URL,

protocol.handle('my-custom-scheme', (request) => {
  if (request.url === 'to-http')
    return net.fetch('https://my-server.com/request')
})

This perhaps addresses the security concern, too: we wouldn't be expanding the attack surface of an existing API that's in use by apps; the existing net.request could remain limited to http/https.

[MarshallOfSound]

Generically 👍 on the idea of net.fetch.

Things to keep in mind:

• Node recently added fetch support as a builtin global, don't think this will get in the way but might be interesting to see what amount of the fetch spec that implements
• Is the intention to support the full Response prototype, e.g. .blob(), .json(), .text(), etc.

[nornagon]

blob doesn’t make sense in the main process context but the other ones do. actually it looks like Node's .blob() functions, so I guess we can too :)

[nornagon]

Ref #36733, it looks like we can probably reuse a bunch of Node's work here and just swap out the actual network stack.

[nornagon]

I'm going to come back to this idea once net.fetch has landed in #36733.

[nornagon]

funny story, the kSupportedProtocols check that currently exists doesn't fire if you pass the url as {url: 'some-protocol://...'}. it does eventually fail further on down the stack, so it's still prevented in the current code, but the check as written in the JS is ineffective.

[nornagon]

I've changed this PR to limit usage of non-http schemes to the new net.fetch, thus ameliorating the above security concerns, and have moved this out of draft.

[itsananderson]

Do we need any documentation changes for this, or is that already covered by the PR to add net.fetch?

[itsananderson]

API LGTM

[jkleinsc]

API LGTM

[jkleinsc]

I'm not sure why the x64 and ia32 Windows tests passed, but the Windows on Arm test failure is legitimate due to different line endings.

[jkleinsc]

API LGTM

[nornagon]

CI failure is flake; merging.

[release-clerk]

Release Notes Persisted

Changed net.fetch to support requests to file: URLs and custom protocols registered with protocol.register*Protocol.