Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open external links in user browser for Theia electron app #13592

Closed
rschnekenbu opened this issue Apr 11, 2024 · 6 comments · Fixed by #13782
Closed

Open external links in user browser for Theia electron app #13592

rschnekenbu opened this issue Apr 11, 2024 · 6 comments · Fixed by #13782
Assignees
Milestone

Comments

@rschnekenbu
Copy link
Contributor

Since #11048, the window open handler for electron application is set to be always a secondary window.
For all external links, we have to rely on the opener-service to delegate properly to an external application with the 'http' open handler (https://github.com/eclipse-theia/theia/blob/master/packages/core/src/browser/http-open-handler.ts).

I would rather expect the Theia electron to open any external links by default with an external browser, and only manually code the opening of a secondary window when required.

This may however cause some issues with security (https://www.electronjs.org/docs/latest/tutorial/security#13-disable-or-limit-navigation)

@tsmaeder
Copy link
Contributor

@rschnekenbu "secondary window" has a technical meaning in Theia. From the the context, I suspect you mean a "new Theia window"?

@tsmaeder
Copy link
Contributor

tsmaeder commented May 8, 2024

@rschnekenbu it's not clear to me what is actually happening. Do you have steps to reproduce?

@tsmaeder
Copy link
Contributor

tsmaeder commented May 8, 2024

Because if I type a https link in an editor (https://github.com) and ctrl-click it, opens in the browser.

@sdirix
Copy link
Contributor

sdirix commented Jun 4, 2024

Hi @tsmaeder,

Within Electron apps, external links are best opened in the system's browser instead of another Electron window. See for example this code which is necessary just to open a link outside of Electron: https://github.com/eclipse-theia/theia-blueprint/blob/bfa92d03e9bf6f572e4410066e856883df5b011f/theia-extensions/product/src/browser/branding-util.tsx#L20-L33

If one renders a normal <a href to some domain, Theia by default will open another Electron window and render it there. It's usually not recommended to do that as the user could navigate to any page then and might visit or be lead to malicious sites which could exploit the "older" Electron Browser.

Typically Electron apps are configured so that any link opens the system's browser and only selected whitelisted ones are allowed to be opened in another Electron window.

@tsmaeder
Copy link
Contributor

tsmaeder commented Jun 5, 2024

I'm just not very sure this is a good idea: I know VS Code does it here, but on the other hand, the electron security doc explicitly warns against it. Enabling opening random links sounds like handing the adopters a big gun to shoot themselves in the foot with. @msujew any input on this one?

@msujew
Copy link
Member

msujew commented Jun 5, 2024

@tsmaeder IMO it would be fine to at least open this up to open all http(s) links via the OS's shell. Maybe show a warning for everything else? I.e. "the application has requested to open <url>. Are you sure you want to open this link using your operating system?". Looking at some of the other Electron software I have on my PC, it seems like everyone just disregards the Electron security advice (especially Microsoft, which is the largest contributor to Electron). MS Teams, VS Code, Slack, etc all just open http links in the browser without hesitation.

tsmaeder added a commit to tsmaeder/theia that referenced this issue Jun 7, 2024
Fixes eclipse-theia#13592

Allow to open https/http links externally and ask the user for all other
protocols.

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
@jfaltermeier jfaltermeier added this to the 1.51.0 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants