[webkit-reviews] review granted: [Bug 218623] Don't treat loopback IP addresses (127.0.0.0/8, ::1/128) as mixed content : [Attachment 413920] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 16 06:56:28 PST 2020


Michael Catanzaro <mcatanzaro at gnome.org> has granted Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 218623: Don't treat loopback IP addresses (127.0.0.0/8, ::1/128) as mixed
content
https://bugs.webkit.org/show_bug.cgi?id=218623

Attachment 413920: Patch

https://bugs.webkit.org/attachment.cgi?id=413920&action=review




--- Comment #15 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 413920
  --> https://bugs.webkit.org/attachment.cgi?id=413920
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413920&action=review

I'm going to give r=me because I don't see any technical problems with the
patch, and I agree with the technical merits of this change. But since Apple
has previously expressed concern with this change -- although I hope such
concerns have been well-addressed by now -- it ought to be approved by an Apple
reviewer as well before landing.

>
LayoutTests/http/tests/security/mixedContent/import-ipv4-loopback-script-in-ifr
ame.html:16
> +window.addEventListener("message", function (error) {
> +    window.data = error.data;
> +    shouldBe(`window.data`, `"TypeError: Cross-origin script load denied by
Cross-Origin Resource Sharing policy."`);
> +    finishJSTest();
> +}, false);

Nice test.

>
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetc
h-mixed-content-to-inscope.https-expected.txt:3
> -PASS Verify Mixed content of fetch() in a Service Worker
> +FAIL Verify Mixed content of fetch() in a Service Worker assert_equals:
expected "finish" but got "FAIL(4)finish"

Hm, I see you reported bug #218795 for this. Interesting. Certainly merits
further investigation.

> Source/WTF/ChangeLog:8
> +	   * Scripts/Preferences/WebPreferencesExperimental.yaml: Introduce a
new experimental
> +	   preference for the new behavior (enabled by default) in case some
ports or users need to
> +	   disable it.

Once upon a time, I successfully proposed a rule that experimental preferences
must not be enabled by default, because if it's enabled by default, it's no
longer experimental. I don't remember that rule ever changing, but it seems
that it's clearly not being followed anymore. So I guess this is OK. I don't
see why it needs to be considered experimental, though: it's a well-understood
change, not a brand new feature that might have bugs. And the preference needs
to exist forever, for use by layout tests. So I think it would make more sense
among non-experimental features.

> Source/WebCore/loader/MixedContentChecker.h:61
> -    static bool isMixedContent(SecurityOrigin&, const URL&);
> +    static bool isMixedContent(const Settings&, SecurityOrigin&, const
URL&);

Nit: consider listing the Settings parameter last, because the SecurityOrigin
and URL are the important parameters that the code is trying to test, while the
Settings are just ancillary data that is used to perform the test. Same for
SecurityOrigin::isAPrioriAuthenticatedURL. (This may be one of my
least-important review comments ever. Who complains about parameter order? :)

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:127
> +    // That's not longer the case after bug 218623, we should probably just
revised how we treat mixed content.

revised -> review

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:132
> -    g_assert_true(test->m_insecureContentDisplayed);
> +    g_assert_false(test->m_insecureContentDisplayed);

Hm, actually, I bet you can save this test for now by changing
WebKitTestServer.cpp to use localhost instead of 127.0.0.1. That should work
for the time being, until the default value of the preference added in bug
#218627 gets changed to true. Maybe we can find time to deprecate and remove
our mixed content handling before then.


More information about the webkit-reviews mailing list