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

Update default BlazorWebView host address #24884

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Sep 23, 2024

Description of Change

Changed the BlazorWebView default host address from 0.0.0.0 to 0.0.0.1 for all platforms (except Tizen).

Another alternative (as mentioned in #24363) was to use a custom host name like blazor, but in my testing this slowed down the initial page load quite a lot due to DNS resolution (in the Maui.Controls.Sample project, load times went from ~450ms to ~2.3 seconds).

This PR initially attempted to change to use localhost, but this causes problems when other processes use localhost simultaneously (e.g., the appium test server). We deemed this to be a risky change.

This PR is effectively an extension of #23906, as it applies a similar fix to Android and Windows platforms.

Issues Fixed

Fixes #24363

@MackinnonBuck MackinnonBuck requested review from a team as code owners September 23, 2024 23:39
@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Sep 25, 2024
@Eilon
Copy link
Member

Eilon commented Sep 26, 2024

Confirmed working on iPhone 14 Simulator with iOS 16.2.

@Eilon
Copy link
Member

Eilon commented Sep 26, 2024

And confirmed iPhone 15 Pro Simulator with iOS 18.0 works as well.

So between the CI automated tests, and a few manual tests, I think we've shown that all platforms are compatible with using 0.0.0.1 as the address!

@Eilon Eilon enabled auto-merge (squash) September 26, 2024 17:30
@@ -49,10 +49,10 @@ internal class WebView2WebViewManager : WebViewManager
// Using an IP address means that WebView2 doesn't wait for any DNS resolution,
// making it substantially faster. Note that this isn't real HTTP traffic, since
// we intercept all the requests within this origin.
internal static readonly string AppHostAddress = "0.0.0.0";
internal static readonly string AppHostAddress = HostAddressHelper.GetAppHostAddress();
Copy link
Member

Choose a reason for hiding this comment

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

This should be a Mapper that users can override

Copy link
Member

Choose a reason for hiding this comment

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

I think the intent here is not to allow complete customization of the address at this point, so we don't need it to be a mapper.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it was just a way in the future I think we can allow users to unblock themselves. and using the "recommended" approach for extensibility, I think we do it on WebView on android from some chrome stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems reasonable, but given the stage in the release cycle, I think we're trying to make this fix as targeted as possible. By adding a new API now, we'd be kind of stuck allowing the configuration of the host address, which adds a new opportunity for misconfiguration. In the chance that this becomes a recurring issue (e.g., if browsers decided to ban 0.0.0.1 next), we could add the mapper at that point. But I'm open to having my mind changed.

@Eilon, do you have any thoughts about this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah definitely a nice feature request. I thin it's already tracked by another issue.

@rmarinho rmarinho disabled auto-merge September 27, 2024 16:45
@Eilon Eilon merged commit cc683f0 into main Sep 27, 2024
97 checks passed
@Eilon Eilon deleted the mbuck/update-default-host-address branch September 27, 2024 19:52
@MackinnonBuck
Copy link
Member Author

/backport to net9.0

Copy link
Contributor

@rmarinho
Copy link
Member

We are going to merge main to net9.0 , just added it there.

rmarinho pushed a commit that referenced this pull request Sep 30, 2024
Update default host address to 0.0.0.1.
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 1, 2024
@angularsen
Copy link

We are having trouble with both 0.0.0.0 and 0.0.0.1 with Stripe Elements UI for collecting payment card details.

  1. Stripe Elements UI's JavaScript does not allow being hosted on IP-based hostnames
  2. Stripe backend does not allow redirecting to IP-based hostnames, to return to the app after adding payment card

Another usecase is locking down google maps API keys to specific domains, we were not able to make this work with 0.0.0.0. I'm sure there are many similar situations with other JavaScript libraries.

We really need this to be configurable to localhost 🙏

@samhouts samhouts added fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-net9.0-nightly This may be available in a nightly release! and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@PureWeen
Copy link
Member

/backport to release/9.0.1xx

Copy link
Contributor

Started backporting to release/9.0.1xx: https://github.com/dotnet/maui/actions/runs/11369775618

Copy link
Contributor

@PureWeen backporting to release/9.0.1xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Update default host address
Using index info to reconstruct a base tree...
M	Microsoft.Maui-dev.sln
M	src/BlazorWebView/src/Maui/BlazorWebView.cs
M	src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs
Falling back to patching base and 3-way merge...
Auto-merging src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs
CONFLICT (content): Merge conflict in src/BlazorWebView/src/SharedSource/WebView2WebViewManager.cs
CONFLICT (add/add): Merge conflict in src/BlazorWebView/src/SharedSource/HostAddressHelper.cs
Auto-merging src/BlazorWebView/src/SharedSource/HostAddressHelper.cs
Auto-merging Microsoft.Maui-dev.sln
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Update default host address
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@PureWeen an error occurred while backporting to release/9.0.1xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

PureWeen pushed a commit that referenced this pull request Oct 16, 2024
Update default host address to 0.0.0.1.
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
@samhouts samhouts added fixed-in-9.0.12 and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change default BlazorWebView local address from 0.0.0.0 to something safer
7 participants