Added the ability to specify a proxy in the config.json file

[TheraNinjaCat]

All that needs to be done to enable a proxy is to specify a key "proxy" in one of the config.json files, and set it to a format accepted by electrons default setProxy function.
i.e. "proxy": "socks5://localhost:8080"

Fixes #735

---

Here's what your changelog entry will look like:

✨ Features

• Added the ability to specify a proxy in the config.json file (#58). Fixes Updater lack proxy configuration #735. Contributed by @TheraNinjaCat.

[t3chguy]

Needs DCO sign-off https://github.com/matrix-org/matrix-js-sdk/blob/master/CONTRIBUTING.rst#sign-off

[TheraNinjaCat]

I assume a comment here works for that too.
Signed-off-by: Greg Best [email protected]

[t3chguy]

Yup, that is fine :)

[jryans]

Looks reasonable to me overall, thanks! 😁

We also document the various config options as well. Could you send a PR to update this also?

[jryans]

You should be able to use await here, and then you can remove the then handler and put loadURL back where it was outside of the if to cover both cases.

[jryans]

Thanks for making this contribution. Upon more reflection with the core team, while we'd like to have proxy support available, we would want to ensure a few things before accepting a PR for it:

• The proxy options should affect the entire app (including Electron main process requests for app update etc.)
• We need some kind of test coverage to ensure the proxy option remains working as code evolves

There's too much risk to privacy and security if a proxy option suddenly stops working and potentially exposes your traffic (if you were using Tor, for example), so it's critical to have test coverage for such a path.

Considering the desktop app currently has no test framework of its own, this could end up being a large task to achieve. If you're still interested in working on it, please reach out in #riot-dev:matrix.org to work through approaches with the team.

[NHAS]

Has there been any more discussion about adding proper proxy support to Element Desktop?

We've been using the --proxy-server=address:port command line option that Electron supports by default, and I imagine that many people who don't have a direct connection (e.g ISP/Work Place level proxy) will be doing the same.

Unfortunately the auto update function does not respect this command line flag and will attempt to connect directly, potentially revealing that the device is using Element/Electron which impacts privacy.
So people who use this flag (as they may have to) might be unknowingly putting themselves at risk.

From a functionality perspective, this also seems rather important to have. This would both enable people who are behind proxies by default to access matrix, and enable greater privacy by allowing less technical people to use proxies.

[kn0wmad]

Has there been any more discussion about adding proper proxy support to Element Desktop?

We've been using the --proxy-server=address:port command line option that Electron supports by default, and I imagine that many people who don't have a direct connection (e.g ISP/Work Place level proxy) will be doing the same.

Unfortunately the auto update function does not respect this command line flag and will attempt to connect directly, potentially revealing that the device is using Element/Electron which impacts privacy.
So people who use this flag (as they may have to) might be unknowingly putting themselves at risk.

From a functionality perspective, this also seems rather important to have. This would both enable people who are behind proxies by default to access matrix, and enable greater privacy by allowing less technical people to use proxies.

Bump. Our users have to use the same workaround and get the same issues.

[TheraNinjaCat]

I’ve not done any changes to this one, it’s been a while and I’ve not kept up to date with the codebase.
The change in the original PR here doesn’t actually fix the auto update proxy leak. As far as I could tell that’s an issue with Electron’s default auto-updater not respecting Electron’s proxy settings. I did look into proposing the replacement that’s suggested in the Electron documentation (that in my testing seemed to fix the issue) but it involved changing more upstream stuff around how updates are handled, however I didn’t feel like a change like that would be received well so I didn’t continue further.

[TheraNinjaCat]

I have revisited this, moved the proxy handling to a helper module, and added basic support for the file: URI scheme.

I have moved the config from config.json to electron-config.json because the former seemed to be more relevant to the vector webapp running inside electron, but proxy settings are more relevant to electron itself.

I have also investigated the autoUpdater as this did not seem to obey the proxy settings from my original implementation, nor the --proxy-server=address:port chromium flag. It turns out the built-in autoUpdater does not support proxying at all, however most electron documentation suggests using the electron-updater instead, which exposes the netSession object which can have proxy settings applied. As far as I can tell these two sessions are the only sessions in the app, however if it turns out that there are more it's trivial to add them to the helper to apply proxy settings to them as well.

The change to electron-updater does potentially break the current build/deploy/auto-update functionality as it no longer uses the same method of checking for updates. Where electron autoUpdater relies on a server responding with specific HTTP status codes, the electron-updater simply deploys a latest-platform.yml file for each platform that contains information for updating.

In regards to a test framework, I don't think the lack of a test suite should hold back implementing this, as due to several comments here people are already using the built-in chromium flag for using a proxy (which coincidentally uses the exact underlying APIs used by my changes), using a proxy isn't necessarily for the intention of increasing privacy as inside a walled-garden network it's often required for access to the internet, and if the underlying APIs stop working in the future, this will be due to an upstream change in electron or chromium.

In terms of writing tests for this, it would be possible to perform tests by using session.resolveProxy(url), and validating that the output matches the defined proxy configuration.

Finally, I have also documented the proxy config inside the README.md file.

[TheraNinjaCat]

I have just noticed that a bunch of packages were updated and I didn't notice before committing, including electron core, which I believe also deprecated enableRemoteModule, hence it being removed. All of those changes don't belong in this PR so after feedback has come back I can revert those changes.

[xanoni]

Can confirm that the package auto updater just doesn't want to use the proxy, so I had to firewall everything that wasn't going to localhost:8118.

I tried using torsocks, proxychains, and I exported the ALL_PROXY and HTTPS_PROXY environment variables .... that all didn't help.

For all other Element traffic, the --proxy-server=127.0.0.1:8118 parameter does the job.

Team should take a fresh look

[turt2live]

this all looks fine, but I'm definitely not a proxy expert. The proxy setup looks fairly complicated for someone to follow though, and I worry that's a barrier to entry when we could/should have our own UI for it.

Further, isn't the system proxy respected, and can't these sorts of options be supplied on the command line already? Not sure I understand the need for the change - tickets would help.

[turt2live]

Not sure if you intended to assign copyright to us or not, but feel free to use your name instead.

[turt2live]

we need to document somewhere that electron-updater is for proxy config, but I don't have any good ideas for where that could be recorded.

[NHAS]

While element itself can use the electron --proxy-server option, this is not applied to the autoupdater which may lead to privacy issues as discussed previously in this issue.
Lacking this functionality also makes it harder to deploy these changes at scale.

For example, imagine a private matrix server that is behind a proxy, currently if we want to set up a number of devices to use it we would have to add a command line argument when Element is executed, which isnt standardised across multiple platforms (Im looking at you Mac).
Even with that argument added the auto updater will still dial out to the internet. Which may be not what users want.

I definitely agree that this should have some UI component to it. Although I would push for the inclusion of at a configuration file option to begin with.
Only partially bias as I want to use it myself 😋

[turt2live]

If we can get that context duplicated to an issue we can reference, that'd be awesome. The PR description currently doesn't mention it, and reading this PR's thread is a bit difficult with limited time :)

[NHAS]

Yup of course. You guys manage a massive amount of comms, so I appreciate that you definitely don't have time to re-read every each time you come back to something.

I'll see if I can bug the maintainer of this issue to add it to the issue description, if that would be sufficient?

[turt2live]

Posting a link here is also just as good - we can get it added.

Looks like someone will need to work on some tests for this though, which is a challenge. If people have good ideas for how to accomplish that, drop by #element-dev:matrix.org on Matrix and we can talk through it.

[TheraNinjaCat]

I appreciate you taking your time to look at this. I'll jump on #element-dev:matrix.org at some point this week when I get a chance to discuss all the points raised, as well as implications of the element-updater change, I do agree this PR has become a bit of a mess, turns out I'm not very good at this whole "git" thing.

[NHAS]

Sweet. I've hijacked an already existing issue that also references issues with the autoupdater:

#735

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dbkr]

This has been sitting since 2021 so I think it's time to close it. This isn't saying, 'no' to the feature, but this PR has 3 years worth of conflicts so it's probably as easy to start afresh anyway.

If anyone picks this up, some points from when we were discussing it:

• An obvious use of this is to put traffic through Tor or similar, so we need to
  1. Make sure it doesn't break (ie. find a way to test it, and test it well)
  2. Make sure all the traffic the app sends uses the proxy. ie. this PR covers the web traffic + auto updater. Is that everything? Do we make any requests from the main process itself? Will we in the future?
• The config passed to this is quite large. It's also just taking the proxy config object and exposing it in the config. Is this interface stable?
• I forget the details of electron auto updating, but having to import a package we previously never pulled in seems very surprising. This would need a lot of explanation in comments as Travis was getting at. What's lacking about the one from electron?
• The PAC file watching is a little 😳. This would definitely need tests. It also feels weird that it's our job.

That said, a good argument for this is that, if, in practice, people are using --proxy-server and leaking update traffic over a clearnet then this will help.