Summary: | Web Inspector: Network: add button to show system certificate dialog | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
Bug Depends on: | 191447 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2018-11-08 22:41:26 PST
Created attachment 354518 [details]
Patch
Comment on attachment 354518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354518&action=review This looks great to me! I think there is a concern around versioning when sending these across systems if they ever change. But as is should work great. > LayoutTests/http/tests/inspector/network/getSerializedCertificate.html:33 > + test(resolve, reject) { > + WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived) > + .then((event) => InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier))) > + .then(resolve, reject); > + > + InspectorTest.evaluateInPage(`createInsecureRequest()`) > + .catch(reject); > + } Write these as async tests? Seems it would read very linearly: async test() { InspectorTest.evaluateInPage(`createInsecureRequest()`); await event = WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived); await InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier))); } > Source/JavaScriptCore/inspector/protocol/Network.json:207 > + { "name": "serializedCertificate", "type": "string" } Might be worth adding a description that this is a base64 encoded WebCore::CertificateInfo. If the CertificateInfo data changes this won't work with remote inspection across systems. For example if inspecting a WebKit version that had a different serialization of CertificateInfo. We might need to version this, or at least make it so `InspectorFrontendHost.showCertificate` will fail and gracefully display a message when it cannot decode the certificate info. > Source/WebCore/platform/network/cf/CertificateInfo.h:162 > + Vector<CFTypeRef, 32> values(size); Hmm, 32 seems like a weird inline capacity to have here, but it's not changing. Comment on attachment 354518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354518&action=review >> LayoutTests/http/tests/inspector/network/getSerializedCertificate.html:33 >> + } > > Write these as async tests? Seems it would read very linearly: > > async test() { > InspectorTest.evaluateInPage(`createInsecureRequest()`); > await event = WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived); > await InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier))); > } I agree that it looks nicer, but I have an aversion to calling code before it's associated listener has been added. I don't have a good reason for this, but it's just something I look out for. If it's any consolation, this is almost linear, in that everything except for the last line is in the order you'd expect (the last line is what kicks off the code, whereas everything before it is setup). >> Source/JavaScriptCore/inspector/protocol/Network.json:207 >> + { "name": "serializedCertificate", "type": "string" } > > Might be worth adding a description that this is a base64 encoded WebCore::CertificateInfo. > > If the CertificateInfo data changes this won't work with remote inspection across systems. For example if inspecting a WebKit version that had a different serialization of CertificateInfo. We might need to version this, or at least make it so `InspectorFrontendHost.showCertificate` will fail and gracefully display a message when it cannot decode the certificate info. As discussed offline, I'll have `InspectorFrontendHost::showCertificate` return true/false depending on whether the deserialization was successful or not. Created attachment 354657 [details]
Patch
Created attachment 354676 [details]
Patch
Comment on attachment 354676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354676&action=review I think this is good, but I have a few questions. > Source/JavaScriptCore/inspector/protocol/Network.json:202 > + "name": "getSerializedCertificate", Nit: This could use a description. Reminder to email our ITMLKit friends. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54 > + static supportsShowingCertificates() > + { > + return window.NetworkAgent && NetworkAgent.getSerializedCertificate && InspectorFrontendHost.showCertificate; > + } I wonder if we should have a `InspectorFrontendHost.supportsShowCertificate()` so that other ports that don't yet have an implementation won't show the "View Certificate" button that doesn't work. > Source/WebInspectorUI/UserInterface/Models/Resource.js:1073 > + if (InspectorFrontendHost.showCertificate(serializedCertificates)) > + return; We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback. > Source/WebKit/UIProcess/RemoteWebInspectorProxy.h:47 > +namespace WebCore { > + > +class CertificateInfo; > + > +} Style: Normally we drop the empty lines. > Source/WebKit/UIProcess/WebInspectorProxy.h:55 > namespace WebCore { > + > +class CertificateInfo; Ditto. > Source/WebKit/UIProcess/mac/WebInspectorProxyMac.mm:428 > > + Style: Extra newline. > Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.h:38 > +namespace WebCore { > + > +class CertificateInfo; > + > +} Ditto. > Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:38 > namespace WebCore { > + > class InspectorController; > +class CertificateInfo; > + > } Ditto. > Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:46 > namespace WebCore { > + > +class CertificateInfo; Ditto. > Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:294 > +#if HAVE(SEC_TRUST_SERIALIZATION) > + [certificatePanel beginSheetForWindow:window modalDelegate:nil didEndSelector:NULL contextInfo:nullptr trust:certificateInfo.trust() showGroup:YES]; > +#else > + [certificatePanel beginSheetForWindow:window modalDelegate:nil didEndSelector:NULL contextInfo:nullptr certificates:(NSArray *)certificateInfo.certificateChain() showGroup:YES]; > +#endif What happens if the certificateChain is empty, or if the trust is NULL (not sure if that can actually happen though). > Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:298 > + [[certificatePanel certificateView] setDisplayDetails:YES]; > + [[certificatePanel certificateView] setDisplayTrust:YES]; > + [[certificatePanel certificateView] setEditableTrust:NO]; You may want to pull the view out into a local to avoid calling certificateView 3 times (it doesn't get optimized out like C++). SFCertificateView *certificateView = [certificatePanel certificateView]; [certificateView setDisplayDetails:YES]; ... It may be worth a comment that we do this after the beingSheetForWindow because the view doesn't exist before hand. Should we also just auto-disclose the details? That seems like it would be useful if the developer is trying to view additional details. https://developer.apple.com/documentation/securityinterface/sfcertificateview/1588428-setdetailsdisclosed?language=objc [certificateView setDetailsDisclosed:YES] > Source/WebKitLegacy/win/WebCoreSupport/WebInspectorClient.h:44 > namespace WebCore { > + Ditto. Comment on attachment 354676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354676&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:1073 >> + return; > > We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback. `InspectorFrontendHost.showCertificate` returns `false` when we were unable to show the certificate, and in that case a message gets added to the console. I'd be fine with adding a `window.alert`, but I wasn't sure about that considering there's no precedent. Created attachment 354838 [details]
Patch
(In reply to Devin Rousso from comment #8) > Comment on attachment 354676 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354676&action=review > > >> Source/WebInspectorUI/UserInterface/Models/Resource.js:1073 > >> + return; > > > > We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback. > > `InspectorFrontendHost.showCertificate` returns `false` when we were unable > to show the certificate, and in that case a message gets added to the > console. I'd be fine with adding a `window.alert`, but I wasn't sure about > that considering there's no precedent. We don't want a window.alert, and that probably won't work as we haven't set up a UIDelegate for that. The console message is probably good enough. Comment on attachment 354838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354838&action=review Everything looks good but you came up with a few ideas (the warning icon if button fails, and IFH.supports). > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54 > + static supportsShowingCertificates() > + { > + return window.NetworkAgent && NetworkAgent.getSerializedCertificate && InspectorFrontendHost.showCertificate; > + } I still think we should have an IFH.supportsShowCertificate so that we don't show the button on ports that it won't work on. > Source/WebInspectorUI/UserInterface/Models/Resource.js:1072 > + await Promise.reject(); // Don't expose the error. Lets change these `await Promise.rejects()` to just `throw undefined` or even just including a error `throw new Error("...");`. > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:121 > + platformShowCertificate(certificateInfo); The bottom of this file needs an empty implementation of this for wincairo, along with the other RemoteWebInspectorProxy::platform* functions. Created attachment 354867 [details]
Patch
Created attachment 354872 [details]
Patch
Comment on attachment 354872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354872&action=review r=me! > Source/WebCore/platform/network/cf/CertificateInfo.h:103 > +namespace WTF { > + > +namespace Persistence { Style: Normally we don't have the extra newline between these (and the closing braces). I see you added them, you may want to remove them. > Source/WebCore/platform/network/cf/CertificateInfo.h:128 > + > + Style: Extra newline, carried from the original but we can clean it up. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54 > + return InspectorFrontendHost.supportsShowCertificate && InspectorFrontendHost.showCertificate && window.NetworkAgent && NetworkAgent.getSerializedCertificate; This isn't actually calling supportsShowCertificate, just checking if it exists which will always be true. Further it looks like we don't have the IFH stub anymore, so we don't need to feature check IFH functions and can assume it always exists. I think this should be: static supportsShowCertificate() { return InspectorFrontendHost.supportsShowCertificate() && window.NetworkAgent && NetworkAgent.getSerializedCertificate; } > Source/WebInspectorUI/UserInterface/Models/Resource.js:1074 > + throw errorString; // Don't expose the error. The comment doesn't seem useful. > Source/WebKit/UIProcess/RemoteWebInspectorProxy.h:117 > + void platformShowCertificate(const WebCore::CertificateInfo&); To get the Windows ports to build you need to include a stub in WebInspectorProxyWPE.cpp and WebInspectorProxyWin.cpp for this. Comment on attachment 354872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354872&action=review >> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54 >> + return InspectorFrontendHost.supportsShowCertificate && InspectorFrontendHost.showCertificate && window.NetworkAgent && NetworkAgent.getSerializedCertificate; > > This isn't actually calling supportsShowCertificate, just checking if it exists which will always be true. > > Further it looks like we don't have the IFH stub anymore, so we don't need to feature check IFH functions and can assume it always exists. > > I think this should be: > > static supportsShowCertificate() > { > return InspectorFrontendHost.supportsShowCertificate() && window.NetworkAgent && NetworkAgent.getSerializedCertificate; > } I made `supportsShowCertificate` an attribute on IFH, so it should be doing logic. Comment on attachment 354872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354872&action=review > Source/WebCore/platform/network/soup/CertificateInfo.h:74 > +template<> struct Coder<GRefPtr<GByteArray>> { Does this need #if USE(GLIB) around it and friends (to line 127). > I made `supportsShowCertificate` an attribute on IFH, so it should be doing logic.
Oh nice! I read the .h and though it was the .idl!
Created attachment 355145 [details]
Patch
Created attachment 355185 [details]
Patch
Created attachment 355187 [details]
Patch
Created attachment 355188 [details]
Patch
Created attachment 355189 [details]
Patch
Try adding include for WebCoreArgumentCoders.h
Comment on attachment 355189 [details] Patch Attachment 355189 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10033238 New failing tests: media/no-fullscreen-when-hidden.html Created attachment 355193 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 355189 [details] Patch Clearing flags on attachment: 355189 Committed r238350: <https://trac.webkit.org/changeset/238350> All reviewed patches have been landed. Closing bug. |