fix: allow requests from electron renderer

[achingbrain]

The Electron Renderer process runs in an embedded browser window so has access to browser-native fetch. If this is used by ipfs-http-client to make requests against the HTTP API, the User-Agent header is set to a value that looks similar to a browser but no Origin or Referer headers are sent and they can't be overridden.

The change here is to relax the user agent check to allow through requests from clients with 'Electron' in their user agents.

[achingbrain]

@aschmahmann does this PR need anything else or can it go in?

[aschmahmann]

Nope, this seems fine. Am I putting this in the next RC?

[achingbrain]

Am I putting this in the next RC?

Yes please!

[lidel]

(cc @rafaelramalho19 @Gozala for visibility and double check of my assumptions)

Hm.. IIUC this PR moves security perimeter a bit and requires more care from other Electron app developers. Namely, introduces a need to ensure there is no loading and executing third-party JS into renderer process.

With this change, in theory, if someone builds/embeds web browser functionality with Electron (https://beakerbrowser.com/ 🙃 ) and user loads a third party page into renderer process on a machine which also happen to have IPFS node running on default ports, this third-party page would have full access to the go-ipfs API, even if the author of Electron app did not intent that to happen.

FYI in ipfs-desktop we use ipfs-http-client outside of a renderer process, and we are able to manually set Origin header in onBeforeSendHeaders hook to match the API port: https://github.com/ipfs-shipyard/ipfs-desktop/blob/3cb6a862f9741f96c28094f493e116bde78e4b86/src/webui/index.js#L141-L144
This way Origin override is a conscious act, scoped to specific BrowserWindow in our app, not global to all Electron apps.

@achingbrain Perhaps instead of removing Origin protection, ipfs-http-client should detect its running in renderer process of Electron and throw an error with link to docs/example how to set proper Origin via webRequest.onBeforeSendHeaders ?

[aschmahmann]

Thanks @lidel. I guess I merged this prematurely. I also noticed playing around with Electron (although it might be that I was doing it wrong) that the origin is file:// by default which (whether intentionally or not) is not an authorized default origin.

[Gozala]

Hm.. IIUC this PR moves security perimeter a bit and requires more care from other Electron app developers. Namely, introduces a need to ensure there is no loading and executing third-party JS into renderer process.

While it does expand margin of error, I am not sure trying to guard against such mistakes is worth it. If electron app is executing third party JS in renderrer process without sandboxing it (e.g. to some origin and no node APIs) that script can access pretty much any node API (including HTTP), disk access etc..

That said I think @lidel is bringing up a really good point, maybe instead of aiming to just have HTTP client work in electron renderrer process we should think through how do we want http-client to be used in Electron and create a guided solution for it (which maybe a documentation, some helper lib or both).

[achingbrain]

1. Sending a fetch request from remote content loaded into in an iframe in the renderer window of electron results in the origin header being set to where the content was loaded from, and CORS checks being followed, eg:

<iframe src='https://evil.com/nasty.html'></iframe>
--
"origin": "https://evil.com"

2. Setting the sandbox attribute to 'allow-scripts' changes the origin to 'null' which will always fail CORS checks, eg:

<iframe sandbox='allow-scripts' src='https://evil.com/nasty.html'></iframe>
--
"origin": "null"

3. Setting the sandbox attribute to 'allow-scripts allow-same-origin' (strongly discouraged in the docs) sets the origin back to where the remote content was loaded from, eg:

<iframe sandbox='allow-scripts allow-same-origin' src='https://evil.com/nasty.html'></iframe>
--
"origin": "https://evil.com"

4. Linking to a page from the main renderer page will replace the app inside electron. fetch requests from a loaded page have an origin and a referer of where the page was loaded from, and can access the node API if nodeIntegration is true for the window. Replacing the app page can be prevented in the main thread by listening for the will-navigate event on the webContents object of the renderer window and cancelling the default action of the event.

No referer header is sent in 1-3.

So with this patch an attack is possible if a node has been configured to allow API requests from an origin that an attacker can serve content from (spinning up a web server on the node itself for example) and the loaded code either replaces the renderer window or is in an unsandboxed iframe or one with permissive sandbox settings.

Essentially these sort of attacks can be mitigated by following existing Electron/Browser security best practices.

[achingbrain]

Something else to consider is that since without this patch any requests made by window.fetch are rejected, it forces the developer to turn nodeIntegration on for the renderer window, exposing the node API which may open up other holes if their renderer thread has no other use for it.

[lidel]

@Gozala and @achingbrain are right (thanks for the detailed analysis!), Electron is a hacky environment where footguns are plenty, and we should not judge this runtime with the same mindset as regular browser context.

Key insight: realistically speaking, without this patch developers would not introduce Origin override in webRequest hook, but would default to using even bigger footguns (disable cross-origin security, and/or expose node APIs in renderer process) to get http client working in Electron.

Hate to say it, but.. sounds like it will be less harmful if we keep this PR in.
Hopefully when ipfs/kubo#1532 is addressed, we can remove this altogether.

[aschmahmann]

Cool, sounds like we're leaving this in. Thanks everyone for putting in the time to look into this, I'll do a release + bubble into go-ipfs later today.