Bug 174992

Created attachment 316856 [details] Patch

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

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

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

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

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

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

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

Created attachment 316873 [details] Patch

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

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

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

Created attachment 316885 [details] Patch

(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 on attachment 316885 [details] Patch Clearing flags on attachment: 316885 Committed r220111: <http://trac.webkit.org/changeset/220111>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33661599>

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