Bug 219257 - [GTK] Allow WebKitTestServer to run non-loopback addresses for API tests
Summary: [GTK] Allow WebKitTestServer to run non-loopback addresses for API tests
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 219160 220922 222273
Blocks: 171934 218623 219198
  Show dependency treegraph
 
Reported: 2020-11-23 06:31 PST by Frédéric Wang (:fredw)
Modified: 2022-01-11 14:27 PST (History)
7 users (show)

See Also:


Attachments
Tentative patch (untested) (4.76 KB, patch)
2020-11-23 06:36 PST, Frédéric Wang (:fredw)
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (reverted) (6.77 KB, patch)
2020-11-24 05:58 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2021-02-24 07:44 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2020-11-23 06:31:35 PST
This is needed to test WEBKIT_INSECURE_CONTENT_RUN and WEBKIT_INSECURE_CONTENT_DISPLAYED APIs if localhost & loopback IP addresses are no longer mixed content. Unfortunately, that does not seem possible until bug 219198 is fixed. Meanwhile, let's try to move from "127.0.0.1" to "localhost" as Michael suggested elsewhere.
Comment 1 Frédéric Wang (:fredw) 2020-11-23 06:36:28 PST
Created attachment 414801 [details]
Tentative patch (untested)
Comment 2 Frédéric Wang (:fredw) 2020-11-23 07:31:20 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

It looks like the API tests pass after this change, so I'm asking review now. Will update the changelog later, but basically this is doing Michael's suggestion in bug 218623 comment 15, so that the test won't be broken when we stop treating loopback IP address as mixed content.
Comment 3 Michael Catanzaro 2020-11-23 08:08:16 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

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

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35
> +        ServerNonLocalHost = 1 << 3,

If you call it ServerNonLoopback instead, this would be a lot less weird. Because passing ServerNonLocalHost to cause the server to use localhost is weird. ;)
Comment 4 Michael Catanzaro 2020-11-23 08:12:00 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

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

LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's fine.

I bet Carlos Garcia will want to review this before it lands.

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38
> -    WebKitTestServer(ServerOptions = ServerHTTP);
> +    WebKitTestServer(uint8_t options = ServerHTTP);

I don't like this... why are you removing its nice type?
Comment 5 Frédéric Wang (:fredw) 2020-11-23 09:15:27 PST
(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 414801 [details]
> Tentative patch (untested)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414801&action=review
> 
> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35
> > +        ServerNonLocalHost = 1 << 3,
> 
> If you call it ServerNonLoopback instead, this would be a lot less weird.
> Because passing ServerNonLocalHost to cause the server to use localhost is
> weird. ;)

Right, but ultimately I want to call it ServerNonLocalHost as we want to stop treating localhost as mixed content too. I'm fine keeping this name for now... but maybe people would add new tests assuming it's just not loopback.

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 414801 [details]
> Tentative patch (untested)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414801&action=review
> 
> LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's
> fine.

Actually three (2 HTTPS and HTTP). I can also change the tests to always use the new version HTTPS version, but not sure that really makes a difference.
 
> I bet Carlos Garcia will want to review this before it lands.
> 
> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38
> > -    WebKitTestServer(ServerOptions = ServerHTTP);
> > +    WebKitTestServer(uint8_t options = ServerHTTP);
> 
> I don't like this... why are you removing its nice type?

ServerOptions is currently an enum of bit flags, so it can't accept a combination ServerHttps | ServerNonLoopback.
Comment 6 Michael Catanzaro 2020-11-23 10:31:43 PST
(In reply to Frédéric Wang (:fredw) from comment #5)
> ServerOptions is currently an enum of bit flags, so it can't accept a
> combination ServerHttps | ServerNonLoopback.

Try: static_cast<ServerOptions>(ServerHttps | ServerNonLoopback)
Comment 7 Michael Catanzaro 2020-11-23 10:32:21 PST
Or better: try std::bitset!
Comment 8 Frédéric Wang (:fredw) 2020-11-24 05:58:55 PST
Created attachment 414835 [details]
Patch (reverted)
Comment 9 Frédéric Wang (:fredw) 2020-11-24 06:01:15 PST
(In reply to Michael Catanzaro from comment #3)
> If you call it ServerNonLoopback instead, this would be a lot less weird.
> Because passing ServerNonLocalHost to cause the server to use localhost is
> weird. ;)

Done

(In reply to Michael Catanzaro from comment #4)
> LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's
> fine.

Merged to use only one HTTPS server, let's see if that does not break any test...

(In reply to Michael Catanzaro from comment #7)
> Or better: try std::bitset!

Done.
Comment 10 EWS 2020-11-26 16:41:21 PST
Committed r270170: <https://trac.webkit.org/changeset/270170>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414835 [details].
Comment 11 WebKit Commit Bot 2021-01-25 07:06:48 PST
Re-opened since this is blocked by bug 220922
Comment 12 Radar WebKit Bug Importer 2021-01-25 07:07:06 PST
<rdar://problem/73570185>
Comment 13 Frédéric Wang (:fredw) 2021-02-22 14:08:12 PST
(In reply to Frédéric Wang (:fredw) from comment #9)
> (In reply to Michael Catanzaro from comment #7)
> > Or better: try std::bitset!
> 
> Done.

This change was problematic, std::bitset's constructor takes a bit mask but its setter/getter relies on the bit's position instead. I've extracted this change to bug 222273.
Comment 14 Frédéric Wang (:fredw) 2021-02-24 07:44:18 PST
Created attachment 421409 [details]
Patch

I tried a bit to rebase, but was not able to make the certificate stuff work. Probably some special config is needed for the domain name...
Comment 15 Frédéric Wang (:fredw) 2021-12-10 02:10:58 PST
Removing myself from assignee since I'm not working on this anymore.