Consider respecting the $BROWSER environment variable on Windows
jrmuizel opened this issue · 4 comments
VS Code uses this environment variable to allow opening a browser over ssh.
webbrowser-rs respects $BROWSER on Linux and works well over a remote connection there. However, on Windows it does not.
It's not supported on Windows because the origin of this is in Unix systems. The problem with blindly extending the same on Windows is that if at all there are any Windows environments which use this envvar in a way differently than on Unix, it can cause bad surprises, or even a security risk. On Unix that isn't an issue because it's a well known envvar.
Can you please help me understand the scenario here? In what way does this crate come into the picture in this VSCode workflow? Also, if at all it's needed to support $BROWSER in Windows, does gating it under a feature suffice (for reasons mentioned above)?
samply uses this crate to open profiles in a webbrowser. When running samply via ssh in a VS Code terminal to unix remote system, VS Code overwrites $BROWSER so that webbrowser-rs will use the local browser instead of trying to use a remote one.
When using ssh in a VS code terminal to a Windows remote system this won't work, because webbrowser-rs doesn't use the $BROWSER that's set by VS Code. Perhaps it makes sense to only use $BROWSER on Windows when being run via ssh (I think this is called a non-interactive session).
It seems samply had walked away from webbrowser to opener crate. From the looks of it, it was to enable Flatpak, which is weird because webbrowser already supported Flatpak by the time of that PR, plus provides a guarantee of opening a browser, which opener does not (say when an editor is configured as default program for html files).
Nevertheless, that PR invalidates the need for this issue, so unless you encountered other scenarios where this is a need, I'd prefer to close this issue. Supporting $BROWSER in an environment where it isn't really standard is a security risk, so I'd want to make this change only if there's an established need.
Closing this issue for now. Please feel free to reopen if you encounter scenarios where this is needed.