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 http origin on Windows, fixes: #3007 #7645

Merged
merged 12 commits into from
Sep 26, 2023

Conversation

FabianLars
Copy link
Member

Sooooo, first i wanted to make it per window but there was one place where i had no idea how to get a per-window config (click on the first commit and search for FIXME:) so i gave up on that for now and switched to a per-app setting on the security config. In theory i would prefer a per-window setting but i also don't think it's worth the headache. Apart from that, the only real annoyence left is convertFileSrc requiring the user to select the scheme rn. I'm not sure if anyone has a better idea that doesn't involve an ipc request, the only thing i have is a init_script that sets a global var on the window object but i'm not sure if that also works for external urls.

Anyway, mostly putting this up to gain feedback (wry isn't realeased yet anyway).

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

@xuchaoqian
Copy link
Contributor

Great job!

per-app setting is enough for most use cases; per-window setting will make things be complicated.

@amrbashir
Copy link
Member

For the convertFileSrc you can use window.location.protocol and it will be automatically correct whether https or http is used

@FabianLars
Copy link
Member Author

That won't be reliable for non-standard origins like the localhost plugin or external urls tho.

@amrbashir
Copy link
Member

That's still fine, you could detect if it is http or https and fallback to https if it is another protocol with an option to explicitly specify whether to use https or http

@FabianLars
Copy link
Member Author

I went for a location.origin check. That's the only thing that came to mind which shouldn't have any false positives or whatever. Checking just for http/https isn't enough since http origins like the ones from the localhost plugin can still use https custom protocols.

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

@FabianLars for the per-window thing, you can trace how window_origin is passed to this prepare_uri_scheme_protocol function https://github.com/tauri-apps/tauri/blob/1.x/core/tauri/src/manager.rs#L703 and do the same, then you can pass it to get_asset and then to set_csp

tooling/api/src/tauri.ts Outdated Show resolved Hide resolved
@FabianLars
Copy link
Member Author

FabianLars commented Aug 29, 2023

for the per-window thing, you can trace how window_origin is passed to this prepare_uri_scheme_protocol function https://github.com/tauri-apps/tauri/blob/1.x/core/tauri/src/manager.rs#L703 and do the same, then you can pass it to get_asset and then to set_csp

Okay, i will have nightmares about the codebase now but at least i know that it's possible now, thanks :D (Edit: srsly i know i'm super full of myself but i still think that it's a bit concerning i didn't see that on third sight)

So the question left is whether we want it to be per-window or per-app. I like the modularity (which is why i started with per-window) but i think realistically nobody will need it and a per-app setting, which iirc was easier on the eyes in the codebase, covers it all. But def open for discussion (aka tell me what to do😊 ) on that one.

@amrbashir
Copy link
Member

I think per-window is better to align with wry implementation and give user flexibility.

@lucasfernog
Copy link
Member

I don't really see the need to have the complexity of the per-window configuration. I would go with the per-app option, and leave it open to be requested by the community.

If we want to be extremely configurable, it would even need to be per-protocol instead.

@lucasfernog lucasfernog marked this pull request as ready for review September 11, 2023 19:09
@lucasfernog lucasfernog requested a review from a team as a code owner September 11, 2023 19:09
@lucasfernog lucasfernog added the security: needs audit This issue/PR needs a security audit label Sep 11, 2023
@tweidinger tweidinger added security: reviewed This issue/PR has been review by wg-security and removed security: needs audit This issue/PR needs a security audit labels Sep 20, 2023
@tweidinger
Copy link
Contributor

I agree with @FabianLars and @amrbashir that having it configurable for specific windows would reduce the impact with multi window applications.
I can imagine that this http scheme use case is not that common but we should consider per-window setting to be in line with upcoming allowlist changes and increased modularity with plugins.

Don't see this as a blocker and the current implementation looks good enough for a first iteration to me 👍

@xuchaoqian
Copy link
Contributor

xuchaoqian commented Sep 20, 2023

We use localhost-plugin to get http worked, but some users can't open our app at all, because of some security policies or other reasons.

This PR comes to the rescue. The current implementation is good enough for us and most use cases.

And we can think of per-app setting as a global setting, and per-window setting overrides the per-app setting. So the current design will still work in the future, IMHO.

@FabianLars
Copy link
Member Author

Soooo, i personally kinda changed my mind here (but also not because it's already a per-app impl) and i think we should not introduce it in v2 at all and have tihs PR's implementation be temporary so per-app would be enough. The reasoning behind not adding it to v2 are:

  1. I don't think there's anything that works on a https scheme but not on a http scheme, and if there is it should be broken on the tauri:// scheme too.
  2. Once we officially drop windows 7 support we can switch to tauri:// on windows too and we always wanted that, right?

Instead we should have what i talked about somewhere before: a good resource requested api users can use to block all http requests or whatever they wanna block.

@lucasfernog-crabnebula
Copy link
Contributor

Sounds good to me @FabianLars

@lucasfernog lucasfernog merged commit 9aa34ad into 1.x Sep 26, 2023
33 of 34 checks passed
@lucasfernog lucasfernog deleted the windows-http-origin branch September 26, 2023 17:40
@Dirreke
Copy link

Dirreke commented Oct 30, 2023

It seems that tauri v1.5 has been merged into v2. But how can I use this feature in v2?

@FabianLars
Copy link
Member Author

@Dirreke The http scheme is now the default in v2. We didn't add a config for it yet because we figured that if http:// doesn't work, tauri:// (used on macos and linux) won't work either. We're not completely opposed to adding it back though but didn't want to do it without it having valid use-cases. Sooo if you do have a requirement for it, please open a seperate issue, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: reviewed This issue/PR has been review by wg-security
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

7 participants