Bug 142378

Created attachment 248030 [details] Patch

Comment on attachment 248030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248030&action=review > Source/WebCore/loader/MixedContentChecker.cpp:68 > + bool allowed = m_frame.settings().allowDisplayOfInsecureContent() || (type == ContentTypeActiveCanWarn); No need for the parentheses here. > Source/WebCore/loader/MixedContentChecker.h:50 > + typedef enum { > + ContentTypeActive, > + ContentTypeActiveCanWarn, > + } ContentType; This should not use typedef. Also it should use enum class: enum class ContentType { Active, ActiveCanWarn };

Committed r181134: <http://trac.webkit.org/changeset/181134>

<rdar://problem/18945975>

Please note that none of the tests test if mixed content is blocked, despite all appearances. I have a patch for this in bug #140940 but I'll need to update it now.

(In reply to comment #5) > Please note that none of the tests test if mixed content is blocked, despite > all appearances. I have a patch for this in bug #140940 but I'll need to > update it now. I think what I want to do is revert the changes this bug made to these tests, and have them explicitly turn off mixed content blocking with testRunner.overridePreference, since the tests need significant changes to test what they now claim to be testing, and I've already written separate tests for that. Easy enough for me to do for WK2, but I don't know how to implement that for WK1. I need to find and modify the equivalent in WK1 of WK2's InjectedBundle::overrideBoolPreferenceForTestRunner() in order to make testRunner.overridePreference work, or the tests will always fail on the WK1 bots. If anyone from Apple could provide guidance on that, would be great. See bug #140940.

(In reply to comment #5) > Please note that none of the tests test if mixed content is blocked, despite > all appearances. I have a patch for this in bug #140940 but I'll need to > update it now. Sorry, why are you saying that? my testing indicated that they are testing the blocking, and the block is happening

(In reply to comment #7) > Sorry, why are you saying that? my testing indicated that they are testing > the blocking, and the block is happening Looking closer at your patch, instead of just the first test I opened up: well you're mostly right. I just checked insecure-css-in-iframe before, since it was first on the list: that one only checks to see whether the content *should* be blocked, i.e. it tests that the MixedContentChecker class works as intended, but it doesn't check to see if the CSS was actually blocked. E.g. in bug #140940 one of my tests loads CSS that turns the background blue, and makes sure the background is white and not blue. But most of your modified tests are fine because they show frame load progress or because of other modifications, like you did to the XHR test: insecure-css-in-iframe was really the exception. We should fix that one, but it's not a big deal. So I was mostly wrong. And we should not necessarily revert these changes, but we should rethink slightly to figure out how much we want to clash with the tests in bug #140940, where I added new tests rather than changing these ones because I wanted to keep the tests for non-blocking mixed content detection: after your change, now we have no tests that detection without blocking works, but it's something we still want to support. (For WebKitGTK+, probably only on a per-host basis: you visit a site and decide you want to load insecure scripts on the site, so you click the shield icon in the address bar to "turn the shield off" -- at least that's the common UI used by our competitors. It sucks to provide this, but I think we need it for the time being. And we must never block mixed content if the TLS connection is unauthenticated, because that would result in quite confusing UI to allow unblocking content: an insecure lock, AND a shield that protects you? I've filed bug #142412 for the GTK+ port for that.) Probably we will want to add at least my tests that aren't redundant with yours, and probably also restore the detected-but-not-blocked tests you "removed" via editing. But it's not a big deal. Anyway, Igalia already paid me to implement this, so let me try to sync up our work. :) I have a tracker bug #140625 for mixed content changes. I will post more there and CC you. I'd appreciate your input on my bugs! One more, unrelated note: for some reason prefetched content is marked as "optionally blockable" (translation: don't block it) in the WIP mixed content spec [1]. It's not clear to me why: seems completely safe to not prefetch stuff, and actually a great idea because then we don't leak cookies to hosts over http:// without the user getting a security warning. The only disadvantage I see is that would be that it will obviously slow things down. So I like your change to block prefetched links, though I worry I don't understand something here as there must be a good reason those are specified as "optionally blockable" instead of "blockable".... [1] http://w3c.github.io/webappsec/specs/mixedcontent

(In reply to comment #5) > Please note that none of the tests test if mixed content is blocked, despite > all appearances. Right, so that was totally wrong: most of them do. :) But the first one I checked did not, so I assumed wrongly.

Comment on attachment 248030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248030&action=review > LayoutTests/http/tests/xmlhttprequest/access-control-response-with-body.html:30 > + setTimeout(function(){ > + if (window.testRunner) > + testRunner.notifyDone() > + }, 100) This is a very strange change, and it made the test flakily fail. 100ms is pretty much never a reasonable value for a timer in a test - and we don't really have any reasonable values beyond 0ms. A test running on a virtual hyperthreaded core can be paused for a lot longer than 100ms, so we should never use "100ms" in a test unless firing too early is OK for some reason.