Bug 191458 – Web Inspector: Network: add button to show system certificate dialog

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

For now, this will only work on macOS using SecurityInterface.

[Radar WebKit Bug Importer]

<rdar://problem/45977019>

[Devin Rousso]

Created attachment 354518 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 354657 [details]
Patch

[Devin Rousso]

Created attachment 354676 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 354838 [details]
Patch

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 354867 [details]
Patch

[Devin Rousso]

Created attachment 354872 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

> 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!

[Devin Rousso]

Created attachment 355145 [details]
Patch

[Devin Rousso]

Created attachment 355185 [details]
Patch

[Devin Rousso]

Created attachment 355187 [details]
Patch

[Devin Rousso]

Created attachment 355188 [details]
Patch

[Devin Rousso]

Created attachment 355189 [details]
Patch

Try adding include for WebCoreArgumentCoders.h

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 355189 [details]
Patch

Clearing flags on attachment: 355189

Committed r238350: <https://trac.webkit.org/changeset/238350>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.