Summary: | Check for SHA1 certificates ignores subresources | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKit2 | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, mcatanzaro, mitz, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=142461 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2015-06-19 13:29:05 PDT
No need for a new function; there is already PageLoadState::didDisplayOrRunInsecureContent. Created attachment 255224 [details]
Patch
The patch is really simple so I think it is safe, but note that this is Mac-specific functionality and I don't have a Mac. Comment on attachment 255224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255224&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:-2947 > - m_pageLoadState.didCommitLoad(transaction, hasInsecureCertificateChain); When hasInsecureCertificateChain was false, this was the code that would reset the hasInsecureContent flag in the page load state when navigating from an “insecure content” page to a “secure” page. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2949 > + m_pageLoadState.didDisplayOrRunInsecureContent(transaction); This is no substitute for that, because it only changes the state from “secure” to “insecure”. Good catch, thanks! I was thinking that during the course of one load, we would never want to change the state from insecure to secure, but of course we need to do so for subsequent loads. The easy fix to my patch is to reset the state to false in didCommitLoad, but that would be more fragile than the current code since the correctness would depend on the order the PageLoadState functions are called inside WebPageProxy::didCommitLoadForFrame. So we should keep the parameter and just rename it to something more accurate. It's hard to think of a good name though, so let's punt that to bug #146157. Created attachment 255265 [details]
Patch
Comment on attachment 255265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255265&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:2944 > + // The page is definitely insecure if true, but could still be insecure even if false. I think I understand this comment, but I am not sure what motivates adding it here now. Doing so doesn’t seem to be directly related to fixing the bug. Created attachment 255289 [details]
Patch
Comment on attachment 255289 [details]
Patch
Thanks for the review. I'll just remove the comment.
Comment on attachment 255289 [details] Patch Clearing flags on attachment: 255289 Committed r185795: <http://trac.webkit.org/changeset/185795> All reviewed patches have been landed. Closing bug. |