Bug 72178

Created attachment 114766 [details] Patch

Can you review this please?

Comment on attachment 114766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114766&action=review We'll also need a test. > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:148 > + if (!m_loader->checkIfDisplayInsecureContent(newRequest.url())) { > + m_loader->cancel(); > + return; > + } Don't we need all the checks from CachedResourceLoader::canRequest? For example, what if this load is forbidden for other reasons?

http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp#L266 Notice that we do a bunch of other checks at the same time as we check for mixed content. If we're missing the mixed content checks, we're probably missing those as well.

(In reply to comment #4) > http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp#L266 > > Notice that we do a bunch of other checks at the same time as we check for mixed content. If we're missing the mixed content checks, we're probably missing those as well. Thanks for the pointers. I figured there might be more to this, but I didn't know where to look. :)

I wonder why the checks aren't applied in the first place. Doesn't the loader use a threadable loader which in turn uses a cached resource loader that should apply the correct checks?

(In reply to comment #6) > I wonder why the checks aren't applied in the first place. Doesn't the loader use a threadable loader which in turn uses a cached resource loader that should apply the correct checks? So it appears that you are correct that AssociatedURLLoader creates a DocumentThreadableLoader which creates a CachedResourceLoader via CachedResourceLoader::requestRawResource(). Since the CachedResourceLoader has a type of RawResource the display checks are skipped. How about this for a fix: 1. Add a MediaResource to CachedResource::Type 2. Update switch() statements in CachedResourceLoader 3. Create the inverse of cachedResourceTypeToTargetType() that maps ResourceRequest::TargetType to CachedResource::Type 4. Update DocumentThreadableLoader::loadRequest() to use the new mapping function to determine what type of CachedResourceLoader() to create instead of always creating a RawResource CachedResource. I believe this should fix the problem and possibly fix any other TargetTypes that happen to be getting loaded through DocumentThreadableLoader.

Adding Nate who added that code path

Not all loads that go through AssociatedURLLoader are for media. Perhaps we need some way to disambiguate what kind of load we're doing.

(In reply to comment #9) > Not all loads that go through AssociatedURLLoader are for media. Perhaps we need some way to disambiguate what kind of load we're doing. I agree. That is why I was suggesting using TargetType to CachedResource::Type translation to differentiate the different types of requests. Only when TargetType is TargetIsMedia will we create a CachedResource with Type MediaResource.

(In reply to comment #10) > (In reply to comment #9) > > Not all loads that go through AssociatedURLLoader are for media. Perhaps we need some way to disambiguate what kind of load we're doing. > > I agree. That is why I was suggesting using TargetType to CachedResource::Type translation to differentiate the different types of requests. Only when TargetType is TargetIsMedia will we create a CachedResource with Type MediaResource. Note that currently TargetType is only available on Chromium, so we can't rely on it for a general solution.

> Note that currently TargetType is only available on Chromium, so we can't rely on it for a general solution. AssociatedURLLoader only exists in Chromium as well. (It's a Chromium API concept.)

(In reply to comment #12) > > Note that currently TargetType is only available on Chromium, so we can't rely on it for a general solution. > > AssociatedURLLoader only exists in Chromium as well. (It's a Chromium API concept.) What about workers? If a worker requests a script, it'll be loaded as CachedRawResource instead of CachedScript, right?

(In reply to comment #13) > (In reply to comment #12) > > > Note that currently TargetType is only available on Chromium, so we can't rely on it for a general solution. > > > > AssociatedURLLoader only exists in Chromium as well. (It's a Chromium API concept.) > > What about workers? If a worker requests a script, it'll be loaded as CachedRawResource instead of CachedScript, right? I believe workers would start using whatever TargetType was specified in CrossThreadResourceRequestData::m_targetType. Right now I believe they are just using RawResource as well. If someone could create me a quick worker example, I'd be happy to investigate more.

Created attachment 116822 [details] Patch

Uploaded a new patch to give people a better idea of what I had in mind. I'm a little nervous about this change since I'm not at all familiar with this code or how wide an impact such a change would be. Expert opinions definitely wanted.

Comment on attachment 116822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116822&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Was it possible to test this? E.g. try to modify one of the tests for mixed content?

(In reply to comment #17) > (From update of attachment 116822 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116822&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > Was it possible to test this? E.g. try to modify one of the tests for mixed content? Haven't gotten to that yet. I wanted to make sure I was on the right track before investing too much effort in polishing the patch. I'll look around the LayoutTests to see if I can find something.

Comment on attachment 116822 [details] Patch Attachment 116822 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10676465

Created attachment 116997 [details] Added Layout Tests

Comment on attachment 116997 [details] Added Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=116997&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:291 > + return static_cast<CachedRawResource*>(requestResource(type, request, String(), options, ResourceLoadPriorityUnresolved, false)); this won't work. If the type is e.g. ImageResource, requestResource will return a CachedResource Maybe CachedRawResource can be smarter about its own type (e.g. move the targetTypeToCachedResourceType to CachedRawResource), and have the cached resource loader query the CachedRawResource for its type before doing its type-based checks?

Created attachment 117282 [details] Only consult targetType() when determining whether it is ok to request the resource.

Comment on attachment 117282 [details] Only consult targetType() when determining whether it is ok to request the resource. Just discovered that redirects to insecure URLs aren't handled by this patch

Created attachment 117479 [details] Fix redirects and added layout tests to verify the fix.

Please take another look. The patch intentionally focuses on just getting canRequest() to do checks with the proper type. I tried various ways of changing type() to return the appropriate type, but various code makes assumptions about (type() == RawResource) => CachedRawResource() and I'd get crashes because of bad casts and various other problems. I believe this is the lowest risk patch to get the desired behavior.

Comment on attachment 117479 [details] Fix redirects and added layout tests to verify the fix. View in context: https://bugs.webkit.org/attachment.cgi?id=117479&action=review > Source/WebCore/ChangeLog:7 > + You could add some text what this patch exactly does

otherwise, looks good

Created attachment 117642 [details] Updated ChangeLog text.

Can I get an r+ & c+ on the latest patch pretty please. :)

Comment on attachment 117642 [details] Updated ChangeLog text. View in context: https://bugs.webkit.org/attachment.cgi?id=117642&action=review Assuming we're not digging ourselves a bigger hole w.r.t. targetType, this looks fine. @jochen? > Source/WebCore/loader/SubresourceLoader.cpp:130 > + CachedResource::Type canRequestType = m_resource->type(); This variable should be a noun. Maybe requestTypeForCanRequest ? > Source/WebCore/loader/SubresourceLoader.cpp:135 > +#if PLATFORM(CHROMIUM) > + if (canRequestType == CachedResource::RawResource) > + canRequestType = CachedResource::targetTypeToCachedResourceType(request().targetType()); > +#endif Is targetType() staying around? I thought there were bug threads about deleting it... Maybe those threads were just about moving it to Chromium specific code? > Source/WebCore/loader/cache/CachedRawResource.h:33 > CachedRawResource(ResourceRequest&); This first constructor should have the explicit keyword.

Comment on attachment 117642 [details] Updated ChangeLog text. View in context: https://bugs.webkit.org/attachment.cgi?id=117642&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:130 >> + CachedResource::Type canRequestType = m_resource->type(); > > This variable should be a noun. Maybe requestTypeForCanRequest ? Done. >> Source/WebCore/loader/SubresourceLoader.cpp:135 >> +#endif > > Is targetType() staying around? I thought there were bug threads about deleting it... Maybe those threads were just about moving it to Chromium specific code? I honestly don't know. I'm not familiar with the history of this code. I just know that TargetType is how the video tag indicates that it is making a request for media. I think trying to change that is much higher risk and will likely trigger several yak shaves. This change at least exposes a use case that needs to be handled when TargetType is eventually removed. >> Source/WebCore/loader/cache/CachedRawResource.h:33 >> CachedRawResource(ResourceRequest&); > > This first constructor should have the explicit keyword. Done

Created attachment 117662 [details] Addressing CR comments

(In reply to comment #31) > (From update of attachment 117642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117642&action=review > > >> Source/WebCore/loader/SubresourceLoader.cpp:135 > >> +#endif > > > > Is targetType() staying around? I thought there were bug threads about deleting it... Maybe those threads were just about moving it to Chromium specific code? > > I honestly don't know. I'm not familiar with the history of this code. I just know that TargetType is how the video tag indicates that it is making a request for media. I think trying to change that is much higher risk and will likely trigger several yak shaves. This change at least exposes a use case that needs to be handled when TargetType is eventually removed. Yeah, I was mostly asking jochen, who was involved in the discussions about the future of targetType().

(In reply to comment #33) > (In reply to comment #31) > > (From update of attachment 117642 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=117642&action=review > > > > >> Source/WebCore/loader/SubresourceLoader.cpp:135 > > >> +#endif > > > > > > Is targetType() staying around? I thought there were bug threads about deleting it... Maybe those threads were just about moving it to Chromium specific code? > > > > I honestly don't know. I'm not familiar with the history of this code. I just know that TargetType is how the video tag indicates that it is making a request for media. I think trying to change that is much higher risk and will likely trigger several yak shaves. This change at least exposes a use case that needs to be handled when TargetType is eventually removed. > > Yeah, I was mostly asking jochen, who was involved in the discussions about the future of targetType(). It's chromium only currently. I still hope that I can make it a general part of loader

> It's chromium only currently. I still hope that I can make it a general part of loader So, Alexey agreed that we don't need to remove it?

Comment on attachment 117662 [details] Addressing CR comments I chatted with jochen. It sounds like this is the right approach for this patch.

(In reply to comment #36) > (From update of attachment 117662 [details]) > I chatted with jochen. It sounds like this is the right approach for this patch. Thanks for the review. I appreciate it. :)

Comment on attachment 117662 [details] Addressing CR comments Clearing flags on attachment: 117662 Committed r101883: <http://trac.webkit.org/changeset/101883>

All reviewed patches have been landed. Closing bug.

New tests introduced in this bug fail on SL, GTK and Qt too: SL fails: - http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r101914%20(35151)/http/tests/security/mixedContent/redirect-http-to-https-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r101914%20(35151)/http/tests/security/mixedContent/insecure-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r101914%20(35151)/http/tests/security/mixedContent/redirect-https-to-http-video-in-main-frame-pretty-diff.html GTK fails: - http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r101898%20(19680)/http/tests/security/mixedContent/redirect-http-to-https-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r101898%20(19680)/http/tests/security/mixedContent/insecure-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r101898%20(19680)/http/tests/security/mixedContent/redirect-https-to-http-video-in-main-frame-pretty-diff.html Qt fails: - http://build.webkit.org/results/Qt%20Linux%20Release/r101902%20(40538)/http/tests/security/mixedContent/redirect-http-to-https-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/Qt%20Linux%20Release/r101902%20(40538)/http/tests/security/mixedContent/insecure-video-in-main-frame-pretty-diff.html - http://build.webkit.org/results/Qt%20Linux%20Release/r101902%20(40538)/http/tests/security/mixedContent/redirect-https-to-http-video-in-main-frame-pretty-diff.html Could you check what happened?

I skipped them on Qt: http://trac.webkit.org/changeset/101918/trunk/LayoutTests/platform/qt/Skipped Please unskip them if the bug is fixed once.

I'm checking the issue on GTK. I think it doesn't work because we don't use the SubResourceLoader for videos.

This caused http://code.google.com/p/chromium/issues/detail?id=106383 (chromium ui_tests WorkerTest.SharedWorkerHttpAuth fails consistently.)

Rebaselined tests introduced by this patch here: http://trac.webkit.org/changeset/101981

Committed r101986: <http://trac.webkit.org/changeset/101986> One more rebaseline since leopard and snowleopard had different results.

Committed r101997: <http://trac.webkit.org/changeset/101997> And one more. Now it has custom expectations on mac-snowleopard and mac-cg-snowleopard only.

Note that the diffs basically say that insecure content is displayed on these ports without a warning :-/

No, for me it looks like these warnings are displayed twice on snowleopard.

Created attachment 118222 [details] Partial revert to restore old behavior for workers.

It appears this change has caused a regression in SharedWorkers (http://crbug.com/106383) and I've been unable to figure out how to fix them over the last to days. Could someone please review the partial revert so that I can at least temporarily restore workers to their old behavior that does less security checks. Thanks.

Comment on attachment 118222 [details] Partial revert to restore old behavior for workers. View in context: https://bugs.webkit.org/attachment.cgi?id=118222&action=review > Source/WebCore/ChangeLog:11 > + No new tests. (OOPS!) Can we not test this problem here? > Source/WebCore/loader/cache/CachedResource.cpp:139 > case ResourceRequest::TargetIsScript: > - return CachedResource::Script; > + // TODO: Change back to CachedResource::Script once http://crbug.com/106383 has been fixed. > + return CachedResource::RawResource; TODO -> FIXME This patch looks wrong on it's face. I'd rather revert the original patch entirely than land this "obviously wrong" code. Can we not just fix the bug directly?

Created attachment 118268 [details] Revert mixed content handling for video fix and follow-up rebases and Skipped updates.

Talked with levin@ and he said he'd help me figure out the issues w/ SharedWorkers. In the mean time I'm just going to revert everything.

Comment on attachment 118268 [details] Revert mixed content handling for video fix and follow-up rebases and Skipped updates. Thanks.

Comment on attachment 118268 [details] Revert mixed content handling for video fix and follow-up rebases and Skipped updates. Clearing flags on attachment: 118268 Committed r102300: <http://trac.webkit.org/changeset/102300>

All reviewed patches have been landed. Closing bug.