Bug 174992 - Layout tests with 'https' suffix should be run over HTTPS
Summary: Layout tests with 'https' suffix should be run over HTTPS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 172930 (view as bug list)
Depends on: 175089
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-31 14:25 PDT by Chris Dumez
Modified: 2017-08-02 14:33 PDT (History)
12 users (show)

See Also:


Attachments
Patch (16.13 KB, patch)
2017-08-01 08:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.65 KB, patch)
2017-08-01 11:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.06 MB, application/zip)
2017-08-01 12:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.79 MB, application/zip)
2017-08-01 12:35 PDT, Build Bot
no flags Details
Patch (15.38 KB, patch)
2017-08-01 12:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-07-31 14:25:26 PDT
WPT tests with 'https' suffix do not appear to be run over HTTPS.
Comment 1 youenn fablet 2017-08-01 08:29:29 PDT
Created attachment 316856 [details]
Patch
Comment 2 Sam Weinig 2017-08-01 08:43:56 PDT
Can we remove the LayoutTests/http/tests/ssl directory now?
Comment 3 youenn fablet 2017-08-01 08:51:38 PDT
(In reply to Sam Weinig from comment #2)
> Can we remove the LayoutTests/http/tests/ssl directory now?

We would need to add https suffix to all http/tests/ssl tests.
Comment 4 Chris Dumez 2017-08-01 08:52:54 PDT
Comment on attachment 316856 [details]
Patch

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

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html:44
> +        internals.setStrictMixedContentMode(false);

I'll check where this test is coming from but something is telling me that we should fix the test rather than disabling mixed content checks.
Comment 5 youenn fablet 2017-08-01 08:53:03 PDT
(In reply to youenn fablet from comment #3)
> (In reply to Sam Weinig from comment #2)
> > Can we remove the LayoutTests/http/tests/ssl directory now?
> 
> We would need to add https suffix to all http/tests/ssl tests.

Given the small number of tests in the ssl folder, we should probably do that and move the corresponding tests to more meaningful folders.
Comment 6 Darin Adler 2017-08-01 08:54:15 PDT
Comment on attachment 316856 [details]
Patch

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

Concept seems fine. But not everything needed is here.

> Source/WebCore/testing/Internals.cpp:3930
> +
> +    auto* frame = document->frame();
> +    if (!frame)
> +        return;
> +
> +    frame->settings().setAllowDisplayOfInsecureContent(true);

Should just be:

    document->settings().setAllowDisplayOfInsecureContent(true);

No need to involve the Frame.

Do we have something that changes this setting back so it won’t affect subsequent tests? I don’t see anything new in this patch, but maybe it already exists?

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html:44
> +    if (window.internals)
> +        internals.setStrictMixedContentMode(false);

This looks wrong. It’s not calling the new function you added. Maybe based on an earlier try?
Comment 7 Darin Adler 2017-08-01 08:55:15 PDT
Comment on attachment 316856 [details]
Patch

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

>> Source/WebCore/testing/Internals.cpp:3930
>> +    frame->settings().setAllowDisplayOfInsecureContent(true);
> 
> Should just be:
> 
>     document->settings().setAllowDisplayOfInsecureContent(true);
> 
> No need to involve the Frame.
> 
> Do we have something that changes this setting back so it won’t affect subsequent tests? I don’t see anything new in this patch, but maybe it already exists?

Oops, I mean:

    document->mutableSettings().setAllowDisplayOfInsecureContent(true);

But also, we should change Document::mutableSettings to just be a non-const overload of Document::settings instead of a separately named function.
Comment 8 Chris Dumez 2017-08-01 08:58:48 PDT
Comment on attachment 316856 [details]
Patch

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

>>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html:44
>>> +        internals.setStrictMixedContentMode(false);
>> 
>> I'll check where this test is coming from but something is telling me that we should fix the test rather than disabling mixed content checks.
> 
> This looks wrong. It’s not calling the new function you added. Maybe based on an earlier try?

This whole test seems to be testing upgrading of insecure requests so disabling mixed content checks likely disables the test in some way.
Comment 9 Chris Dumez 2017-08-01 09:02:43 PDT
Comment on attachment 316856 [details]
Patch

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

>>>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html:44
>>>> +        internals.setStrictMixedContentMode(false);
>>> 
>>> I'll check where this test is coming from but something is telling me that we should fix the test rather than disabling mixed content checks.
>> 
>> This looks wrong. It’s not calling the new function you added. Maybe based on an earlier try?
> 
> This whole test seems to be testing upgrading of insecure requests so disabling mixed content checks likely disables the test in some way.

FYI, this test is actually a web-platform-test. It is upstream at:
upgrade-insecure-requests/iframe-upgrade.https.html

We should make sure it is up-to-date.
Comment 10 Chris Dumez 2017-08-01 09:03:55 PDT
(In reply to Chris Dumez from comment #9)
> Comment on attachment 316856 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316856&action=review
> 
> >>>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html:44
> >>>> +        internals.setStrictMixedContentMode(false);
> >>> 
> >>> I'll check where this test is coming from but something is telling me that we should fix the test rather than disabling mixed content checks.
> >> 
> >> This looks wrong. It’s not calling the new function you added. Maybe based on an earlier try?
> > 
> > This whole test seems to be testing upgrading of insecure requests so disabling mixed content checks likely disables the test in some way.
> 
> FYI, this test is actually a web-platform-test. It is upstream at:
> upgrade-insecure-requests/iframe-upgrade.https.html
> 
> We should make sure it is up-to-date.

WebKit seems to pass all checks for upstream's:
https://w3c-test.org/upgrade-insecure-requests/iframe-upgrade.https.html

So it is fails locally, it likely means the test is out of date or not setup properly?
Comment 11 Daniel Bates 2017-08-01 10:43:16 PDT
The WPT aspect of this bug is a duplicate of bug #172930.
Comment 12 Daniel Bates 2017-08-01 10:49:02 PDT
Comment on attachment 316856 [details]
Patch

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

>>> Source/WebCore/testing/Internals.cpp:3930
>>> +void Internals::disableMixedContentChecks()
>>> +{
>>> +    auto* document = contextDocument();
>>> +    if (!document)
>>> +        return;
>>> +
>>> +    document->setStrictMixedContentMode(false);
>>> +
>>> +    auto* frame = document->frame();
>>> +    if (!frame)
>>> +        return;
>>> +
>>> +    frame->settings().setAllowDisplayOfInsecureContent(true);
>> 
>> Should just be:
>> 
>>     document->settings().setAllowDisplayOfInsecureContent(true);
>> 
>> No need to involve the Frame.
>> 
>> Do we have something that changes this setting back so it won’t affect subsequent tests? I don’t see anything new in this patch, but maybe it already exists?
> 
> Oops, I mean:
> 
>     document->mutableSettings().setAllowDisplayOfInsecureContent(true);
> 
> But also, we should change Document::mutableSettings to just be a non-const overload of Document::settings instead of a separately named function.

As mentioned to Youenn in person today. If we decide to toggle setAllowDisplayOfInsecureContent() in the test then it is unnecessary to add a new Internals function as setAllowDisplayOfInsecureContent is defined in Settings.in so we generate an Internals settings function for it. We can toggle this setting in a test using window.internal.settings.setAllowDisplayOfInsecureContent(...).
Comment 13 Daniel Bates 2017-08-01 10:50:04 PDT
(In reply to Daniel Bates from comment #12)
> We can toggle this setting in a test using window.internal.settings.setAllowDisplayOfInsecureContent(...).

*window.internals.settings.setAllowDisplayOfInsecureContent(...)
Comment 14 youenn fablet 2017-08-01 11:03:04 PDT
Created attachment 316873 [details]
Patch
Comment 15 Build Bot 2017-08-01 12:20:02 PDT
Comment on attachment 316873 [details]
Patch

Attachment 316873 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4235253

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html
Comment 16 Build Bot 2017-08-01 12:20:04 PDT
Created attachment 316881 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-08-01 12:35:19 PDT
Comment on attachment 316873 [details]
Patch

Attachment 316873 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4235218

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/iframe-upgrade.https.html
Comment 18 Build Bot 2017-08-01 12:35:21 PDT
Created attachment 316882 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 youenn fablet 2017-08-01 12:51:05 PDT
Created attachment 316885 [details]
Patch
Comment 20 youenn fablet 2017-08-01 12:58:26 PDT
(In reply to youenn fablet from comment #19)
> Created attachment 316885 [details]
> Patch

Moving to internalSettings so no need to modify Internals.
Also, InternalSettings are reset for each test.

Since this test is coming from Chromium and Chromium is exporting these tests to wpt, we should get rid of this one and use WPT ones instead.
Comment 21 WebKit Commit Bot 2017-08-01 14:02:40 PDT
Comment on attachment 316885 [details]
Patch

Clearing flags on attachment: 316885

Committed r220111: <http://trac.webkit.org/changeset/220111>
Comment 22 WebKit Commit Bot 2017-08-01 14:02:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2017-08-01 14:03:27 PDT
<rdar://problem/33661599>
Comment 24 Daniel Bates 2017-08-01 14:48:37 PDT
*** Bug 172930 has been marked as a duplicate of this bug. ***