WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
57195
Include certificate when sending a WebCore::ResourceError to UI process on Windows
https://bugs.webkit.org/show_bug.cgi?id=57195
Summary
Include certificate when sending a WebCore::ResourceError to UI process on Wi...
Jeff Miller
Reported
2011-03-27 16:38:52 PDT
In order to display an invalid certificate, the UI process needs access to the certificate from the web process on Windows. Keep track of the certificate in the WebCore::ResourceError and add support for encoding this in the message that's sent to the UI process.
Attachments
Patch
(904.51 KB, patch)
2011-03-27 17:17 PDT
,
Jeff Miller
no flags
Details
Formatted Diff
Diff
Patch
(885.16 KB, patch)
2011-03-28 10:45 PDT
,
Jeff Miller
aroben
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Jeff Miller
Comment 1
2011-03-27 17:17:19 PDT
Created
attachment 87092
[details]
Patch
Adam Roben (:aroben)
Comment 2
2011-03-28 06:01:52 PDT
Comment on
attachment 87092
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=87092&action=review
Seems worth another go-round. But this looks great!
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:142 > + CFErrorRef cfError = resourceError.cfError(); > + if (!cfError) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + RetainPtr<CFDictionaryRef> userInfo(AdoptCF, CFErrorCopyUserInfo(cfError)); > + if (!userInfo) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + PCCERT_CONTEXT certificate = static_cast<PCCERT_CONTEXT>(wkGetSSLPeerCertificateDataPtr(userInfo.get())); > + if (!certificate) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + }
Why can't we just use ResourceError::m_certificate here?
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:152 > + CertFreeCertificateContext(reinterpret_cast<PCCERT_CONTEXT>(ptr));
static_cast should work fine.
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:158 > + PCCERT_CONTEXT certCopy = 0;
You can wait to declare this until you initialize it on line 165.
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:164 > + if (!certDealloc) { > + CFAllocatorContext allocContext = { > + 0, 0, 0, 0, 0, 0, 0, deallocCertContext, 0 > + }; > + certDealloc = CFAllocatorCreate(kCFAllocatorDefault, &allocContext); > + }
Having a separate createCertContextDeallocator function would clean this up a bit. Then you could just assign its result into the certDealloc variable and not need an if.
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:187 > + resourceError = WebCore::ResourceError(domain, errorCode, failingURL, localizedDescription, copyCert(certificateChain[0]));
I personally prefer .first() instead of [0].
Jeff Miller
Comment 3
2011-03-28 10:43:50 PDT
(In reply to
comment #2
)
> (From update of
attachment 87092
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=87092&action=review
> > Seems worth another go-round. But this looks great! > > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:142 > > + CFErrorRef cfError = resourceError.cfError(); > > + if (!cfError) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + RetainPtr<CFDictionaryRef> userInfo(AdoptCF, CFErrorCopyUserInfo(cfError)); > > + if (!userInfo) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + PCCERT_CONTEXT certificate = static_cast<PCCERT_CONTEXT>(wkGetSSLPeerCertificateDataPtr(userInfo.get())); > > + if (!certificate) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > Why can't we just use ResourceError::m_certificate here?
Good idea, I added a public certificate() method so we can do that. I also addressed all your other comments in the patch I'm about to upload.
Jeff Miller
Comment 4
2011-03-28 10:45:40 PDT
Created
attachment 87164
[details]
Patch
WebKit Review Bot
Comment 5
2011-03-28 10:48:12 PDT
Attachment 87164
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 WebKitLibraries/ChangeLog:15: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Adam Roben (:aroben)
Comment 6
2011-03-28 10:59:56 PDT
Comment on
attachment 87164
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=87164&action=review
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:130 > + CFErrorRef cfError = resourceError.cfError(); > + if (!cfError) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + }
I don't think this is needed anymore.
> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:144 > + CFDataRef certificateData = resourceError.certificate(); > + if (!certificateData) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + PCCERT_CONTEXT certificate = reinterpret_cast<PCCERT_CONTEXT>(CFDataGetBytePtr(certificateData)); > + if (!certificate) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + encoder->encode(WebKit::PlatformCertificateInfo(certificate));
Maybe ResourceError should have an accessor that returns a PCCERT_CONTEXT? And/or maybe we should have a function that returns a PlatformCertificateInfo given a ResourceError? What you have now seems fine, too, but these other functions might make it cleaner.
Jeff Miller
Comment 7
2011-03-28 11:04:26 PDT
I filed
https://bugs.webkit.org/show_bug.cgi?id=57250
for the check-webkit-style failure, which I think is a false positive.
Jeff Miller
Comment 8
2011-03-28 11:09:28 PDT
(In reply to
comment #6
)
> (From update of
attachment 87164
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=87164&action=review
> > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:130 > > + CFErrorRef cfError = resourceError.cfError(); > > + if (!cfError) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > I don't think this is needed anymore. > > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:144 > > + CFDataRef certificateData = resourceError.certificate(); > > + if (!certificateData) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + PCCERT_CONTEXT certificate = reinterpret_cast<PCCERT_CONTEXT>(CFDataGetBytePtr(certificateData)); > > + if (!certificate) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + encoder->encode(WebKit::PlatformCertificateInfo(certificate)); > > Maybe ResourceError should have an accessor that returns a PCCERT_CONTEXT? And/or maybe we should have a function that returns a PlatformCertificateInfo given a ResourceError? What you have now seems fine, too, but these other functions might make it cleaner.
PlatformCertificateInfo is part of WebKit, and ResourceError is in WebCore, and I didn't want to cross the streams. However, I agree I could make this better, which I'll look at doing in another patch.
Jeff Miller
Comment 9
2011-03-28 11:19:34 PDT
Committed
r82137
: <
http://trac.webkit.org/changeset/82137
>
WebKit Review Bot
Comment 10
2011-03-28 12:00:49 PDT
http://trac.webkit.org/changeset/82137
might have broken Leopard Intel Release (Tests)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug