non-HTTP(s) URLs now works with start #14370

anomius · 2024-11-17T23:40:57Z

Description

this PR should close #14315
This PR enhances the start command in Nushell to handle both files and URLs more effectively, including support for custom URL schemes. Previously, the start command only reliably opened HTTP and HTTPS URLs, and custom schemes like Spotify and Obsidian which were not handled earlier.

Custom URL Schemes Support:

Added support for opening custom URL schemes, controlled via the ALLOWED_SCHEMES environment variable.
Users can now specify additional allowed schemes, e.g., spotify, obsidian, mailto, etc.

Security Checks:

Implemented security checks to prevent opening URLs with disallowed schemes.
By default, only http and https schemes are allowed.
Users receive an informative error if they attempt to open a URL with a disallowed scheme.

Warnings for Unusual Schemes:

Added warnings when opening URLs with schemes other than http and https.
Alerts users to exercise caution when opening links with unusual schemes.

Detailed Error Messages:

Improved error reporting for failed external commands.
Captures and displays error output from the system to aid in debugging.

Example

Opening a custom URL scheme (e.g., Spotify):

$env.ALLOWED_SCHEMES = "http,https,spotify"
start spotify:track:4PTG3Z6ehGkBFwjybzWkR8?si=f9b4cdfc1aa14831

Opens the specified track in the Spotify application.

User-Facing Changes

New Feature: The start command now supports opening URLs with custom schemes specified in the ALLOWED_SCHEMES environment variable.
Security Enhancement: By default, only http and https schemes are allowed to prevent unintended behavior.
Warning Messages: Users receive warnings when opening URLs with unusual schemes to promote caution.

Tests Added:

test_load_allowed_schemes_from_env_with_value checks that the environment variable is correctly read and parsed.
test_load_allowed_schemes_from_env_without_value ensures default schemes are used when the variable is unset.
test_url_allowed_scheme verifies that URLs with allowed schemes are processed.
test_url_disallowed_scheme checks that URLs with disallowed schemes are rejected.

NotTheDr01ds · 2024-11-18T23:39:20Z

Thanks for the ping on this. I totally missed it!

So ... I was probably wrong about a simple environment variable being able to do this. Is there anything to prevent malicious code from doing something like:

$env.ALLOWED_SCHEMES = "http, https, bad"
start bad://rm -rf /

That said, I'm still trying to figure out if this is really a problem. PowerShell doesn't seem to restrict the URL schemes that can be opened. If an application is installed in Windows to handle the scheme, I think start-process in PowerShell will launch it.

I'm just not sure yet.

If you did need to restrict it, you would have to set this in $env.config instead of a simple environment variable, I think. The $env.config (at the moment) can't be changed after parsing starts, as far as I can tell.

Or maybe ask the user for confirmation before launching?

Again, I'm not sure any of this is necessary given the behavior of other platforms, but I'd definitely want to hear more opinions on it.

anomius · 2024-11-19T09:33:16Z

i think that $env.config is a good approach ,but
for users just getting dotfiles and script that can setup shortcuts to different applications using the protocol $env.ALLOWED_SCHEMES should be good option ,that it will just run
also like you mentioned that start-process in PowerShell so this might not be a high/critical security risk
and asking the user for confirmation before launching could get annoying for scripted shortcuts and routines

WindSoilder · 2024-11-19T12:11:41Z

Is it necessary to control the allowed scheme with $env.ALLOWED_SCHEMES?
What about allow all schemes, but it's required to be a url. I don't think something like this is allowed: start spotify:track:4PTG3Z6ehGkBFwjybzWkR8?si=f9b4cdfc1aa14831

NotTheDr01ds · 2024-11-19T13:05:15Z

Is it necessary to control the allowed scheme with $env.ALLOWED_SCHEMES?

I don't see any benefit to it, based on my above comment.

What about allow all schemes, but it's required to be a url. I don't think something like this is allowed: start spotify:track:4PTG3Z6ehGkBFwjybzWkR8?si=f9b4cdfc1aa14831

That would limit the usefulness a bit. The original Discord discussion was about launching application links.

At least Obsidian (from the original discussion) uses the :// form.

What would be the benefit of disallowing the Spotify form?

Side-note: PowerShell's start-process launches that Spotify link. Rick-rolled, yes, I would have been, if my volume hadn't been muted. But ... is that a problem?

I think the tl;dr here is that application links are just a natural part of all platforms at this point. If the application to handle them is on the system, there's not much we can do at that point, right?

WindSoilder · 2024-11-20T11:26:14Z

That would limit the usefulness a bit. The original Discord discussion was about launching application links.
At least Obsidian (from the original discussion) uses the :// form

Yeah that's the point. I think start should open local path or a url, spotify:track:4PTG3Z6ehGkBFwjybzWkR8?si=f9b4cdfc1aa14831 is not a valid urI syntax

anomius · 2024-11-20T19:33:30Z

Is it necessary to control the allowed scheme with $env.ALLOWED_SCHEMES?

I don't see any benefit to it, based on my above comment.

However, adding a precautionary check could be worthwhile. This would ensure users are aware when they’re adding schemes to the allowed list. While it’s not a stringent security measure, it could serve as useful bookkeeping.

Rick-rolled, yes, I would have been, if my volume hadn't been muted. But ... is that a problem?

Apologies for the rick-roll 😅 — never expected anyone to actually try it!

I think start should open local path or a URL

start should support the entire URI scheme, not just URLs,This would make it far more versatile and useful.

WindSoilder · 2024-11-21T12:56:38Z

start should support the entire URI scheme, not just URLs,This would make it far more versatile and useful.

I'm still not convinced. For the following reasons:

URI schemes have many formats. I'm not sure if it's good to support all of them.
More importantly, open crate itself only supports URLs, not URI

NotTheDr01ds · 2024-11-22T16:29:07Z

@WindSoilder Are you sure? I just cargo install'd the open crate and successfully rickrolled myself using your spotify:track:4PTG3Z6ehGkBFwjybzWkR8?si=f9b4cdfc1aa14831 application link above.

WindSoilder · 2024-11-22T22:56:49Z

@NotTheDr01ds TBH I haven't play with open crate a lot.
I get the information from it's doc and it only mentioned supporting URL and mentioned nothing with URI, so maybe we shouldn't rely on it.

NotTheDr01ds · 2024-11-23T00:51:10Z

I see that there aren't any examples in the crate doc for this, but:

The repo says:
Use this library to open a path or URL using the program configured on the system. It is equivalent to running one of the following:
```
# macOS
$ open <path-or-url>
# Windows
$ start <path-or-url>
# Linux
$ xdg-open <path-or-url> || gio open <path-or-url> || gnome-open <path-or-url> || kde-open <path-or-url> || wslview <path-or-url>
```
I don't think I'd be too concerned about the <path-or-url> description here, since most (all?) of those commands support application schemes. If we truly want to confirm, I guess we could ask on the repo.
That's not just a "it works like these apps" - Internally it's just passing the arg to those commands.

I think it's fair to say that our start implementation is just using those commands "behind the scenes" (via the open crate) and is subject to any limitations (or features) in those helpers. For instance, on WSL, our open command makes use of the wslview command because the underlying xdg-open does.

Doesn't it make sense to allow our implementation to take advantage of the behavior of those commands without limiting it to URLs?

IanManske · 2024-11-28T18:22:05Z

Rather than a environment variable, would it make sense to have a flag instead? I.e., --allow-schemes (-s): list<string>.

Also, I think it would be safe to add the file://scheme to the default allow list.

One more thing, I don't think it's necessary to have the alerts for unusual schemes. We will already require users to pass a flag or set an environment variable for unusual schemes.

fdncred · 2024-11-30T14:22:45Z

where are we at with this?

IanManske · 2024-11-30T18:36:32Z

Just waiting for response / fixes for my previous comment.

anomius · 2024-12-03T19:09:29Z

Rather than a environment variable, would it make sense to have a flag instead? I.e., --allow-schemes (-s): list<string>.

Also, I think it would be safe to add the file://scheme to the default allow list.

this looks good. I will add these changes

NotTheDr01ds · 2024-12-04T14:06:30Z

Rather than a environment variable, would it make sense to have a flag instead? I.e., --allow-schemes (-s): list<string>.

Yikes - Please no! IMHO, flags should only be for exceptions to the standard startup. If a user always wants to allow a certain scheme, then they would always have to start with the flag. I run nu so many times a day that this would be unbearable.

That said, I'm not sure that an environment variable is "safe" here either.

One more thing, I don't think it's necessary to have the alerts for unusual schemes. We will already require users to pass a flag or set an environment variable for unusual schemes.

Agreed - If we can figure out how to whitelist safely and conveniently.

IanManske · 2024-12-09T00:10:36Z

Actually, now I'm not sure if we even need the allow list for schemes. If I understand correctly, the application or scheme handler has the majority of the responsibility to ensure that opening a link does not do destructive or modifying actions. Or, if it does, then to prompt the user for confirmation or require some sort of authentication in the URL. I.e., if you have installed an application and have set it as the default handler for a scheme, then you are trusting that application to correctly and securely handle URLs with that scheme from any source.

This is why the underlying OS openers (start, open, xdg-open) do not have an allow list (I think). Rather, the list of registered scheme handlers acts as the global allow list. So, I'm not sure that we need to add an explicit allow list for start when the underlying OS openers don't do so on top of the list of registered handlers. For example, some versions of xdg-mime have this text in the man page:

Requests to make an application a default handler may be subject to system policy or approval by the end-user. xdg-mime query can be used to verify whether an application is the actual default handler for a specific file type.

Security Note: Never set a handler that will blindly execute code or commands from the file being handled. Such behaviour will sooner than later lead to unintended code execution i.e. through a curious user trying to inspect a freshly downloaded file but running it by accident.

Keeping opening and executing separate actions helps with people protecting themselves from malware, the default handler is an opener, not a runner.

But I might be wrong on this. @anomius mentioned in the issue that there are some potentially problematic schemes like "control://;rm -rf /. and changing browser settings". I couldn't find any information on this, so I just want to confirm whether these are issues.

NotTheDr01ds · 2024-12-10T19:57:11Z

Actually, now I'm not sure if we even need the allow list for schemes.

That's pretty much the conclusion I came to.

I'm not sure about the PR itself, though - Would we be susceptible to badbatbut if we allowed this?

IanManske · 2024-12-15T19:54:19Z

Yeah, but that can be fixed in a follow up PR (or in this PR).

anomius · 2024-12-16T19:43:53Z

Thanks for all the input. It seems like the consensus is that maintaining an explicit allow list for URL schemes isn’t really necessary. The OS and registered application handlers already provide a gatekeeping mechanism, and other shells or system commands don’t impose their own restrictions on schemes. Given this, I’m inclined to simplify the PR:
• Remove the ALLOWED_SCHEMES environment variable and the related checks.
• Drop the additional warnings for unusual schemes.
• Just let start work like the OS open/start/xdg-open commands, opening any URI that the system knows how to handle.

We could introduce an optional, user-defined blacklist that resides in their Nushell config. This would let advanced users explicitly prevent certain schemes from being opened if they’re concerned about specific protocols.

anomius · 2025-01-22T13:13:23Z

Hey, I was away on holiday, but I’ve now pushed the changes. If you could please review and merge them, that would be great.

IanManske

Thanks for the update! Just a few minor things and then it should be good to go!

IanManske · 2025-01-23T05:32:38Z

crates/nu-command/src/filesystem/start.rs

+        let path_buf = Path::new(&path_no_whitespace).to_path_buf();
+        let full_path = cwd.join(&path_buf);
+        // Check if the path exists or if it's a valid file/directory
+        if full_path.exists() || path_buf.components().count() == 1 {


What is the path_buf.components().count() == 1 for exactly? We already checked if full_path.exists().

IanManske · 2025-01-23T05:32:57Z

crates/nu-command/src/filesystem/start.rs

+            .item
+            .trim_end_matches(|x| matches!(x, '\x09'..='\x0d'))
+            .to_string();
+        // Load allowed schemes from environment variable


Suggested change

// Load allowed schemes from environment variable

IanManske · 2025-01-23T05:34:19Z

crates/nu-command/src/filesystem/start.rs

+            .trim_end_matches(|x| matches!(x, '\x09'..='\x0d'))
+            .to_string();


Suggested change

.trim_end_matches(|x| matches!(x, '\x09'..='\x0d'))

.to_string();

.trim_end_matches(|x| matches!(x, '\x09'..='\x0d'));

IanManske · 2025-01-23T05:34:53Z

crates/nu-command/src/filesystem/start.rs

+        let cwd = engine_state.cwd(Some(stack))?;
+        let path_buf = Path::new(&path_no_whitespace).to_path_buf();
+        let full_path = cwd.join(&path_buf);


Suggested change

let cwd = engine_state.cwd(Some(stack))?;

let path_buf = Path::new(&path_no_whitespace).to_path_buf();

let full_path = cwd.join(&path_buf);

let cwd = engine_state.cwd(Some(stack))?;

let full_path = cwd.join(path_no_whitespace);

Refactor path handling in Start command to simplify whitespace trimming and path resolution - Removed unnecessary conversion to `String` after trimming whitespace from the input path. - Streamlined the path resolution process by directly joining the current working directory with the trimmed path. - Enhanced readability and maintainability of the code. This change improves the handling of file paths in the Start command, ensuring that whitespace is properly managed without redundant operations.

Refactor whitespace handling in Start command - Simplified the trimming of whitespace from the input path by removing unnecessary conversions. - Improved path resolution by directly joining the current working directory with the trimmed path. - Enhanced code readability and maintainability. This change ensures better handling of file paths in the Start command.

Refactor whitespace handling and path resolution in Start command - Simplified the trimming of whitespace from the input path by removing unnecessary conversions. - Improved path resolution by directly joining the current working directory with the trimmed path. - Enhanced code readability and maintainability. This change ensures better handling of file paths in the Start command.

IanManske

Great, thanks for the PR! Merging 🎉

tmillr · 2025-01-24T02:48:09Z

So ... I was probably wrong about a simple environment variable being able to do this. Is there anything to prevent malicious code from doing something like:
$env.ALLOWED_SCHEMES = "http, https, bad"
start bad://rm -rf /
That said, I'm still trying to figure out if this is really a problem. PowerShell doesn't seem to restrict the URL schemes that can be opened. If an application is installed in Windows to handle the scheme, I think start-process in PowerShell will launch it.

If some running code already has the privilege to run system commands and do start bad://rm -rf /, then it may as well just do rm -rf / directly? Why would it need to use start at all? Therefore I don't think limiting URL schemes accomplishes anything here. Once you have access to a shell, any command can already be run (as allowed by OS-level user/file perms)?

And if the OS is elevating privileges when/prior to opening a URL, then that's on the OS (either an OS setting needs to be changed, or it's a security bug in the OS).

NotTheDr01ds · 2025-01-29T13:58:06Z

If some running code already has the privilege to run system commands

Yes, we reached agreement that this was not a concern.

So happy to have this landed!

anomius added 2 commits November 18, 2024 05:07

non-HTTP(s) URLs now works with start

fa30b52

ci cd push

ad48a81

Merge branch 'nushell:main' into main

8f298cd

anomius added 3 commits January 22, 2025 18:09

Merge branch 'nushell:main' into main

da302f1

Worked on this user is in control

0a08c7f

solved fmt issues

1889e47

IanManske reviewed Jan 23, 2025

View reviewed changes

anomius added 3 commits January 23, 2025 18:32

IanManske approved these changes Jan 24, 2025

View reviewed changes

IanManske merged commit fd684a2 into nushell:main Jan 24, 2025
15 checks passed

github-actions bot added this to the v0.102.0 milestone Jan 24, 2025

fdncred added the pr:commands This PR changes our commands in some way label Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non-HTTP(s) URLs now works with start #14370

non-HTTP(s) URLs now works with start #14370

anomius commented Nov 17, 2024 •

edited

Loading

NotTheDr01ds commented Nov 18, 2024 •

edited

Loading

anomius commented Nov 19, 2024

WindSoilder commented Nov 19, 2024

NotTheDr01ds commented Nov 19, 2024 •

edited

Loading

WindSoilder commented Nov 20, 2024

anomius commented Nov 20, 2024 •

edited

Loading

WindSoilder commented Nov 21, 2024 •

edited

Loading

NotTheDr01ds commented Nov 22, 2024

WindSoilder commented Nov 22, 2024

NotTheDr01ds commented Nov 23, 2024

IanManske commented Nov 28, 2024 •

edited

Loading

fdncred commented Nov 30, 2024

IanManske commented Nov 30, 2024

anomius commented Dec 3, 2024

NotTheDr01ds commented Dec 4, 2024

IanManske commented Dec 9, 2024

NotTheDr01ds commented Dec 10, 2024

IanManske commented Dec 15, 2024

anomius commented Dec 16, 2024

anomius commented Jan 22, 2025

IanManske left a comment

IanManske Jan 23, 2025

IanManske Jan 23, 2025

IanManske Jan 23, 2025

IanManske Jan 23, 2025

IanManske left a comment

tmillr commented Jan 24, 2025 •

edited

Loading

NotTheDr01ds commented Jan 29, 2025

		.trim_end_matches(\|x\| matches!(x, '\x09'..='\x0d'))
		.to_string();

non-HTTP(s) URLs now works with start #14370

non-HTTP(s) URLs now works with start #14370

Conversation

anomius commented Nov 17, 2024 • edited Loading

Description

NotTheDr01ds commented Nov 18, 2024 • edited Loading

anomius commented Nov 19, 2024

WindSoilder commented Nov 19, 2024

NotTheDr01ds commented Nov 19, 2024 • edited Loading

WindSoilder commented Nov 20, 2024

anomius commented Nov 20, 2024 • edited Loading

WindSoilder commented Nov 21, 2024 • edited Loading

NotTheDr01ds commented Nov 22, 2024

WindSoilder commented Nov 22, 2024

NotTheDr01ds commented Nov 23, 2024

IanManske commented Nov 28, 2024 • edited Loading

fdncred commented Nov 30, 2024

IanManske commented Nov 30, 2024

anomius commented Dec 3, 2024

NotTheDr01ds commented Dec 4, 2024

IanManske commented Dec 9, 2024

NotTheDr01ds commented Dec 10, 2024

IanManske commented Dec 15, 2024

anomius commented Dec 16, 2024

anomius commented Jan 22, 2025

IanManske left a comment

Choose a reason for hiding this comment

IanManske Jan 23, 2025

Choose a reason for hiding this comment

IanManske Jan 23, 2025

Choose a reason for hiding this comment

IanManske Jan 23, 2025

Choose a reason for hiding this comment

IanManske Jan 23, 2025

Choose a reason for hiding this comment

IanManske left a comment

Choose a reason for hiding this comment

tmillr commented Jan 24, 2025 • edited Loading

NotTheDr01ds commented Jan 29, 2025

anomius commented Nov 17, 2024 •

edited

Loading

NotTheDr01ds commented Nov 18, 2024 •

edited

Loading

NotTheDr01ds commented Nov 19, 2024 •

edited

Loading

anomius commented Nov 20, 2024 •

edited

Loading

WindSoilder commented Nov 21, 2024 •

edited

Loading

IanManske commented Nov 28, 2024 •

edited

Loading

tmillr commented Jan 24, 2025 •

edited

Loading