Summary: | ContentType::ActiveCanWarn should not allow bypassing allowDisplayOfInsecureContent setting | ||
---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> |
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | ap, cgarcia, darin, fred.wang, mcatanzaro, oliver, pnormand |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Linux | ||
Bug Depends on: | |||
Bug Blocks: | 140625 |
Description
Michael Catanzaro
2015-03-08 17:42:21 PDT
(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. |