Since bug #142378, if MixedContentChecker::canDisplayInsecureContent is called with ContentType::ActiveCanWarn then the content is unconditionally allowed, regardless of the browser's mixed content settings. This means that content of type ContentType::ActiveCanWarn can no longer be blocked. We want to support blocking all mixed content, since this is needed for privacy-focused web browsers, so got to check a setting. But which setting? There are several options we can pick from: * We could give up on the semantics of display/run and use display for all content we do not want to block by default, and run for content that we do want to block by default. The security community lingo for this is "passive" and "active" content which sort of maps to the names "display" and "run", but these names are not good because you're hardly "running" a script or a font. Then by default we would disallow "running" of insecure content (including fonts, frames, etc.) but allow "display". This was what I chose to do, but the result is confusing because the allowRunningOfInsecureContent setting now prevents the display of much content. Not a good option because "run" and "display" don't mean what you think they do. * We can add a third setting to determine how much content to block (strict mode blocks all displayed content, lenient mode allows display of images and videos and "running" of forms). Then by default we would disallow both "display" and "running" of insecure content, but turn off strict mode. That's not a good option because then setting allowDisplayOfInsecureContent and allowRunningOfInsecureContent settings to false will still allow displaying and running some insecure content. * We can have the canDisplayInsecureContent function check the allowRunningOfInsecureContent setting unless it is of type ActiveCanWarn. Then we can allow "display" but not "running" of insecure content by default, but MixedContentChecker::canDisplayInsecureContent would check the allowRunningOfInsecureContent setting half the time. This one is really confusing! * Deprecate both existing settings so they do nothing and start over with a clean slate. Remove canDisplayInsecureContent and canRunInsecureContent from MixedContentChecker, and the corresponding functions from FrameLoaderClient. Then we can have just one InsecureContentAllowed setting with the possible values "all" "some" "none" defaulting to "some", and everything will actually make sense. (We could also map the old settings to the new setting, such that display but not run -> "some".) This is the only option I can think of that's not confusing, but I suspect that is not permissible? Any other ideas? I see no good options; got to pick one though! I think option two is probably the least confusing of the first three, but I don't think the benefit is really worth adding a new setting. Related: we should also rename ContentType::ActiveCanWarn to ContentType::OptionallyBlockable, since it is currently only used for passive content. That naming looks like it was just a think-o.
(In reply to comment #0) > these names are not good because you're hardly "running" a script or a > font Um, I meant "you're hardly 'running' a frame" (or a web socket or something else we want to block by default besides a script), not "you're hardly 'running' a script."
I don't understand why but note that the current patch for bug 218623 is changing the expectation: g_assert_true(test->m_insecureContentDisplayed); If you have any idea why and how the test should really be updated, suggestions are welcome :-)
Well, insecure content is no longer displayed, because you change loopback/localhost addresses to be trusted. So a test that ensures the insecure content warning is emitted when loading from loopback/localhost is expected to fail after your change. I think the only way to update the test would be to expose an API setting to toggle whether loopback/localhost addresses are trusted or not, as you've done for TestController. That would be overkill: it doesn't make sense to add public API solely for use by tests. So let's not do that. Alternativs: * Perhaps it might be possible to allow API tests to change TestController settings? I don't think any tests currently attempt this, and I'm not sure how hard it would be. * You could simply give up and remove the affected API test. But then we would have no tests for the insecure-content-detected signal. * Finally, the best option, but also the hardest option, would be to modernize how WebKit handles mixed content to match Chrome's current behavior, which is better than ours. Nowadays, Chrome has started blocking all mixed content unconditionally (except form targets, but it will block those too very soon), per https://www.feistyduck.com/bulletproof-tls-newsletter/issue_70_chrome_developers_want_to_eliminate_mixed_content. If we were to implement that, then we could deprecate the insecure-content-detected signal and remove the API tests for it. The modern behavior would be to automatically try to rewrite the URL to https://, and allow the content to fail to load if that doesn't work. (You could make exceptions for localhost and loopback.) Of course, that would be quite a bit of additional work, and would require rebaselining all the mixed content tests, but you would then be able to immediately close this bug, and probably many of the other bugs marked against bug #140625. (Lastly, I'll mention that there's no longer much point in trying to fix this particular bug in 2020. It would be easier to implement Chrome's behavior.)
Thanks Michael, (In reply to Michael Catanzaro from comment #3) > Well, insecure content is no longer displayed, because you change > loopback/localhost addresses to be trusted. So a test that ensures the > insecure content warning is emitted when loading from loopback/localhost is > expected to fail after your change. OK, I just read https://webkit-search.igalia.com/webkit/source/Source/WebKit/UIProcess/API/gtk/WebKitWebView.h#144 and I understand better what the test is doing now. "insecure content run" or "insecure content displayed" was a bit confusing to me, I understood that this means they were *not* blocked. > I think the only way to update the test > would be to expose an API setting to toggle whether loopback/localhost > addresses are trusted or not, as you've done for TestController. That would > be overkill: it doesn't make sense to add public API solely for use by > tests. So let's not do that. > * Perhaps it might be possible to allow API tests to change TestController > settings? I don't think any tests currently attempt this, and I'm not sure > how hard it would be. Last week I quickly checked the API tests and didn't find any preference change in GTK. iOS/macOS do some changes and at least iOS has one API test failing, so I need to investigate this anyway. > * You could simply give up and remove the affected API test. But then we > would have no tests for the insecure-content-detected signal. > * Finally, the best option, but also the hardest option, would be to > modernize how WebKit handles mixed content to match Chrome's current > behavior, which is better than ours. That would be ideal but much bigger than bug 171934. I'm trying to do it progressively as this is touching a lot of tests and I'm not sure all ports can enable the behavior change. So I guess if changing the option does not work, I would disable the test temporarily until the rewrite.
@Michael: The patch for bug 413920 is ready. I ended just changing the expectation of the test and adding a FIXME. Not ideal, but I understand from your reply that we want to get rid of this API in the long term anyway. And trying to align progressively with https://w3c.github.io/webappsec-mixed-content/ is a step toward that goal. We could expose a new API like I had done for frame flattening, but that seems worse (note: on iOS I was able to modify the preference before running the API tests, but we don't seem to have such a generic way to set preferences on GTK).
(In reply to Frédéric Wang (:fredw) from comment #5) > @Michael: The patch for bug 413920 is ready. That's not the right bug number. What's the right bug number? :)
(In reply to Michael Catanzaro from comment #6) > (In reply to Frédéric Wang (:fredw) from comment #5) > > @Michael: The patch for bug 413920 is ready. > > That's not the right bug number. What's the right bug number? :) sorry it's bug 218623 (I realized after posting but thought "well michael will figure out")
This is obsoleted by bug #219396 and bug #247197.