Bug 174992 – Layout tests with 'https' suffix should be run over HTTPS

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

WPT tests with 'https' suffix do not appear to be run over HTTPS.

[youenn fablet]

Created attachment 316856 [details]
Patch

[Sam Weinig]

Can we remove the LayoutTests/http/tests/ssl directory now?

[youenn fablet]

(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.

[Chris Dumez]

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.

[youenn fablet]

(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.

[Darin Adler]

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?

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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?

[Daniel Bates]

The WPT aspect of this bug is a duplicate of bug #172930.

[Daniel Bates]

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(...).

[Daniel Bates]

(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(...)

[youenn fablet]

Created attachment 316873 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 316885 [details]
Patch

[youenn fablet]

(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.

[WebKit Commit Bot]

Comment on attachment 316885 [details]
Patch

Clearing flags on attachment: 316885

Committed r220111: <http://trac.webkit.org/changeset/220111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33661599>

[Daniel Bates]

*** Bug 172930 has been marked as a duplicate of this bug. ***