fix: don't bypass redirect checks

[nornagon]

Description of Change

Remove unneeded bypass of redirect checks when creating URL loader factories.

Checklist

• PR description included and stakeholders cc'd
• npm test passes

Release Notes

Notes: none

[deepak1556]

Originally added in #19338

We have a comment about not performing redirect checks

electron/shell/browser/net/proxying_url_loader_factory.cc

Line 264 in 947f1b0

// Note: In Electron we don't check IsRedirectSafe.

unlike the upstream version https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc;l=1397

@zcbenz do you remember why redirect checks were skipped ? Seems like current test suite passes when checks are enabled

[zcbenz]

There was some limitation in chromium network code that only a few hard-coded url schemes can be redirected to, which broke custom schemes in Electron. But if the tests are passing with the the bypass removed, then the limitation should have been gone.

[release-clerk]

No Release Notes

[trop]

I have automatically backported this PR to "21-x-y", please check out #35366

[trop]

I have automatically backported this PR to "18-x-y", please check out #35367

[trop]

I have automatically backported this PR to "20-x-y", please check out #35368

[trop]

I have automatically backported this PR to "19-x-y", please check out #35369

[KishanBagaria]

This has broken some behavior for us, a 302 redirect from http://localhost:1234 to a data/file URI results in net::ERR_UNSAFE_REDIRECT.

is it possible to revert to the original behavior with a flag? Tried webSecurity: false but that doesn't have any effect.

[nornagon]

@KishanBagaria can you expand on why you need that redirect to work? You should be able to work around it if you need by detecting the redirect in the main process and manually loading the new URL I think.

[KishanBagaria]

We run a localhost server (in a worker_thread) that serves some SVG images (data: URI) over a 302 redirect, this should be easy to fix by returning the data directly instead.

Another route redirects to a file: URI. Altho it's possible to use fs.createReadStream and pipeline to the response, it'd be more efficient if the renderer process reads the file directly.

[deepak1556]

@KishanBagaria why are using localhost server for page load instead of custom protocol ? Custom protocol are powerful enough to support any resource scenario and also will help you align with the security checks of chromium.

[KishanBagaria]

@deepak1556 we run a worker_thread to not block the CPU and electron APIs (for custom protocol) aren't available there.

Edit: for more context, we don't load regular HTML pages with the localhost server. There's a HTTP asset route that dynamically loads data and either returns a buffer or redirects to the file:// URI if the asset is cached on disk. We could use a file proxy route that does await stream.promises.pipeline(fs.createReadStream(filePath), res) but that's less than ideal since the renderer is capable of reading from file:// URIs directly.

[zcbenz]

Redirecting from http to file would trigger security checks in lots of places in Chromium, it is actually surprising to me that this has been working. But on the other hand I think we should revert this change as it is breaking existing behavior.

[MarshallOfSound]

@zcbenz the change in behavior is intentional and this change should not be reverted

[KishanBagaria]

We're going to be stuck on an older Electron version bc of this for a while and I'm sure more silent people will be too. I think having a flag (perhaps global) would be better than reverting for all since it's for security.

[MarshallOfSound]

and I'm sure more silent people will be too.

Can't prove it, but the usecase of redirecting to a data / file URI from an http server must be beyond edge-case. You shouldn't even be running a localhost server in your electron app anyway let alone violating web standards and serving up that redirect.

The workaround is a small refactor (serving the data uri contents directly or streaming the file). I don't support exposing a flag for this and we will not be reverting it.

[deepak1556]

@KishanBagaria in your case the dynamic asset which you want to serve from disk can instead be provided via a custom protocol with standard privilege and registered as a file protocol provider https://github.com/electron/electron/blob/main/docs/api/protocol.md#protocolregisterfileprotocolscheme-handler . You will only need to provide the file path as response from the handler, all of the reading is delegated to chromium using the same code path as file://. Ideally, I would suggest avoiding using a local http server in your application but if you need it, redirect from http -> custom protocol for a resource is okay. The only forbidden resource redirects are the following https://source.chromium.org/chromium/chromium/src/+/main:content/public/common/url_utils.cc;l=72-94

Also, protocol handlers which delegate to chromium are usually non-expensive code paths, but if you see them blocking the main process then please measure it and file an issue, we can look into it. Thanks!

[KishanBagaria]

Thanks, will look into that. I'll make the http server in worker thread redirect to fileproxy: instead of file: and register fileproxy: custom protocol to redirect to file: / serve file. (Edit: can confirm this works well)
This should be better than streaming.

Fwiw, WKWebView also has the same restriction for http: to file: redirects but not for http: to data: redirects.

[mosaice]

registerSchemesAsPrivileged was also affected

This example works on "19.0.13"
https://gist.github.com/mosaice/4dce470b7b46527d29fd8f5135baeca4

I found that other people had similar problems
see #35581