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

feat(core): allow window label pattern for remote domain access scope #7468

Conversation

james138138
Copy link

@james138138 james138138 commented Jul 21, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@james138138
Copy link
Author

This should resolve #7461

@james138138
Copy link
Author

Doc PR is tauri-apps/tauri-docs#1330

@james138138 james138138 marked this pull request as ready for review July 21, 2023 22:13
@james138138 james138138 requested a review from a team as a code owner July 21, 2023 22:13
@wusyong wusyong added the status: needs review A maintainer must review this code label Jul 23, 2023
@lucasfernog
Copy link
Member

I've pushed some changes to enhance this but they are breaking changes (good thing this PR targets v2 anyway). If we want to backport this for v1, we need to stick to your original PR.

Thanks for your contribution! Let's wait for a security audit on this.

@lucasfernog lucasfernog added security: needs audit This issue/PR needs a security audit and removed status: needs review A maintainer must review this code labels Jul 24, 2023
@lucasfernog lucasfernog requested a review from a team July 24, 2023 13:10
@james138138
Copy link
Author

@lucasfernog Thanks for the updating, there was a build error, I've pushed a commit, please take a look. Thanks.

{
let windows = Option::<Vec<String>>::deserialize(deserializer)?.unwrap_or_default();
for w in &windows {
if w == "*" || w == "**" {
Copy link
Contributor

@tweidinger tweidinger Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is not sufficient as devs can just write ?* or [?*] to bypass this full wildcard prevention check.
Also from my perspective allowing ** anywhere doesn't make sense. The glob crate is imho meant for file paths and we should use a more simpler approach here.

From my perspective we should use simple string comparisons like starts_with or ends_with to allow something like foo- or -remote for labels instead of using complex matching pattern. We could expose these two patterns via a flag (e.g: "fuzzy_matching" : true)

If there is a reason to go for regex patterns I'm all ears but my assumption is most devs want to allow remote access to certain window types where they can prefix with remote-. Using the glob crate would allow for more fine grained matching but will be a footgun for most devs.

@tweidinger tweidinger added security: feedback loop wg-security added some feedback but no conclusion based on feedbac and removed security: needs audit This issue/PR needs a security audit labels Oct 24, 2023
@lucasfernog
Copy link
Member

We redesigned the whole remote URL scope on #8428

Thank you for this PR!

@lucasfernog lucasfernog closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: feedback loop wg-security added some feedback but no conclusion based on feedbac
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants