-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(core): allow configuring remote domains with IPC access, closes #5088 #5918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking on this challenge. I left some comments, some are blocking IMO.
Apart from those comments, I remember there was a reason why I didnt reuse the same function to inject the scripts in the window, but I don't remember why...
core/tauri-utils/src/config.rs
Outdated
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
pub struct ExternalCommandAccessScope { | ||
/// The url to allow, matched against the webview URL using a glob pattern. | ||
pub url: Url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I used a string here is to allow URL patterns. In our case we need to allow all URLs (*
) but there is a more "legitimate" use case to allow all sub domains of a particular domain (*.example.com
). Since it is converted into a glob anyway down the line it is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since in the general use case this is extremely fragile, it should be hard to allow all domains. Non pentest/security apps are the exception with edge cases and deliberately 'insecure' settings.
core/config-schema/schema.json
Outdated
@@ -2547,6 +2557,37 @@ | |||
} | |||
] | |||
}, | |||
"ExternalCommandAccessScope": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if the name should change since it also whitelist plugin
core/tauri-utils/src/config.rs
Outdated
/// loaded malicious or compromised sites could start executing commands on the user's device. | ||
/// | ||
/// This configuration allows a set of external urls to have access to the Tauri commands. | ||
/// Wildcards patterns are accepted and can be used to allow either all paths or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK if you use an URL, this is not true anymore. But like I said I prefer the glob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should still work, we use the same logic for the HTTP allowlist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to test but I am pretty certain the Url parser rejects a single *
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but you can do https://*
(or https://**
?) with effectively the same effect.
Edit: stuff like https://*.domain.com
also works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is good for official TaurI, wont cut it for us since we also need to support http but I will maintain my fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, you can also do http://...
of course. Sure, it's a bit annoying if you want to support both protocols at the same time because it only accepts a single url (especially if we take into account the other convo about finer allowlist-like control)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, thus my recommendation to keep it as a free string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing http
is super dangerous. And we could also allow an array of urls on the same scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean sure but at the same time this feature is an escape hatch and its made abundantly clear with the name that its dangerous. Http is perfectly acceptable for a lot of use cases like local installations or forwarded via an SSH tunnel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about only allowing https://
in the normal configuration while you could still manipulate the state (and scope) from rust core if you really really need to and understand the implications?
The same goes for wildcards like https://*
and only allow https://example.tld/*
or https://*.example.tld
this way the already insecure feature is still somewhat reasonable.
invoke.resolver.reject("Scope not defined"); | ||
return Ok(()); | ||
} | ||
|
||
if let Some(module) = &payload.tauri_module { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm I don't like that. Currently there is no check for modules like fs
. I think we should do something similar to plugin where we can whitelist each module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about adding an allowlist configuration object for each external access scope, but maybe the root allowlist can be used for that instead? Any reason a Tauri app should have multiple allowlists, one for each URL?
Also this can be confusing since you'd need to enable the allowlist on the root object to add it to the binary, and also allow it in the external access scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is important since it relates to security. Let me give you our usecase.
We have a first in-app window that manages the connections of the user. That window has access to the filesystem to store the configuration.
The user the clicks on a connection which opens a second window with the external URL. We really do not want this window accessing the filesystem directly. We just want it to access certain carefully crafted commands that are safe to call.
This is why I originally only allowed commands and even then I wanted to build a way to limit which commands could be called
Though I agree there could be a default to allow it to take the same values as the general allow list for modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should add an option to block all Tauri APIs. That's easy and won't bloat the codebase with runtime allowlist checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a change to address this.
invoke.resolver.reject("Plugin not allowed"); | ||
return Ok(()); | ||
} | ||
} | ||
manager.extend_api(invoke); | ||
} else { | ||
manager.run_invoke_handler(invoke); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we forked Tauri to ship our patched version, I had some ideas. I think whitelisting which command can be run would be really cool, but not a blocker for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the message above. Also, it would need a lot of code to add this :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can come in a subsequent PR, happy to pitch in for the implementation
This PR opens up some really cool possibilities. Also, I know it's probably still a WIP. After trying it out I think that the messaging and/or error related to the Originally had something like this in my "dangerousExternalCommandAccess": [
{
"url": "https://my-localhost-app-url",
"windows": ["my-app-name"]
}
] When changed to Not sure whether it's a matter of improving the |
@austenc we do mention it's the window labels, see https://github.com/tauri-apps/tauri/pull/5918/files#diff-ff73997dde3dc4965aa0c59ce0b37c7d208ae5b6b7686bf19e14d93c98cc1bacR1116. I'll improve the error message though. |
Thanks for helping out a rust noob :) And thanks again for the work on this feature, really great! |
How do I exactly use this tauri pull request? But when I run
Which makes sense, because the NPM tauri-cli library is still the v1.2.2. I don't know much about NPM, so I was wondering how I can use this pull request now? |
@1zun4 see https://tauri.app/v1/guides/faq#how-can-i-use-unpublished-tauri-changes ( Oh and make sure to also use the |
required pull request tauri-apps/tauri#5918
Thank you very much! It works flawless with my application. Looking forward for it being merged.
... took me a while to figure it out... haha |
required pull request tauri-apps/tauri#5918
Hello, is there any way to modify/add dynamically a value to the dangerousExternalCommandAccess field from the main.rs ? I would need, for example, to get my remote server address from another configuration file, a environment variable or a register key. |
What's the status of this PR? |
Looks like there is lots of interest in this feature. What can the community to do help it land? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, this is an approach that removes security features that underly the entire premise of Tauri - all of us can agree on this. It is basically permitting people to drive a motorcycle without a helmet.
To that extent I would propose that we empower the individual to KNOW that this is going on and DENY such permissive use, especially in "*" wildcard situations.
One idea to harden this is to use a model similar to the way our Isolation pattern works, where the app sends an authorization to the remote, and the remote has to reply properly or tauri IPC execution is prevented.
I think @tauri-apps/wg-security must absolutely sign off on this before it enters canon, especially since this modifies the isolation.js guardrails as well.
The security team has been auditing this PR and the finds will be public soon - along with the proposed solution to improve security. |
Can't wait for their input, I still stand with the review I made 3 months ago. This feature is an escape hatch for "dangerous" usecases, so for me it should allow users to whitelist each element as they wish. It should be very specific in its definition but broad in what it allows (like: I want to allow plugin X for all websites). It should definitely not be used 99% of the time. Our usecase for it was to be able to allow loading remote UI that can interact with some whitelisted tauri commands. It would even be ok with a compiler warning if this feature is used. |
One thing I did not touch is usage of remote URLs with the isolation protocol enabled. Since we can't inject the isolation protocol scripts (?) it does not work at all right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor documentation changes but implementation looks good to me 👍
@tillmann-crabnebula I've pushed some changes to make this work with the isolation pattern. I'm planning on merging this and release 1.3 next week if we can get all blog posts ready. |
We had to change the approach here from "configuring remote URLs with glob patterns" to "configuring domains" for security reasons. If you need to allow ALL URLs, that is super dangerous and you'll need to do so by listening to the navigation event via |
core/tauri-utils/src/config.rs
Outdated
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
pub struct ExternalCommandAccessScope { | ||
/// The url to allow, matched against the webview URL using a glob pattern. | ||
pub url: Url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since in the general use case this is extremely fragile, it should be hard to allow all domains. Non pentest/security apps are the exception with edge cases and deliberately 'insecure' settings.
core/tauri-utils/src/config.rs
Outdated
/// loaded malicious or compromised sites could start executing commands on the user's device. | ||
/// | ||
/// This configuration allows a set of external urls to have access to the Tauri commands. | ||
/// Wildcards patterns are accepted and can be used to allow either all paths or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about only allowing https://
in the normal configuration while you could still manipulate the state (and scope) from rust core if you really really need to and understand the implications?
The same goes for wildcards like https://*
and only allow https://example.tld/*
or https://*.example.tld
this way the already insecure feature is still somewhat reasonable.
Thats fair, at least there is a way to do it. I will give it a try this week. When is it planned to be released? |
We're working to release it this week. |
huuray |
A regression introduced in the last commit from #5918.
Hello, will this be released for v1.x or will this have to wait until v2.0? |
It will be part of v1.3 |
taruri 优先使用 pwa 应用 tauri-apps/tauri#5918
Implemented upgrade to Tauri version 1.3, eliminating the need for utilizing the "feat/remote-ipc" branch. Instead, we can now leverage the newly introduced "dangerousRemoteDomainIpcAccess" option in the "tauri.conf.json" file, which was introduced in version 1.3.0. For further details, please refer to the relevant pull request at the following link: tauri-apps/tauri#5918. While this feature has undergone testing and is confirmed to be functional, it is imperative to conduct further testing to ensure its reliability. If any issues are encountered during usage, kindly report them for further investigation and resolution. Your feedback is greatly appreciated.
Can we have this feature in the |
@luucasrb It already is in the next branch. iirc it was also part of alpha.9 but not 100% sure about that. |
Thanks, you are right. My confusion was that the |
I want to add domains inside on_navigation, but I just don't succeed. Can you give me an example, how I can do it? |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___
)Other information