Bug 230173 – Web Inspector: Network: "Lock" icon next to domain name in table view should show security information

• Component: Web Inspector
• Status: NEW

[Patrick Angle]

Clicking the lock icon should take you to that resource/request's Security sub-tab to view details about the certificate for the resource/request.

[Radar WebKit Bug Importer]

<rdar://problem/82992637>

Currently working on this - would it not be preferable for the click to bring up the certificate directly (similar to clicking on the lock in the URL area) instead of the Security sub-tab?

[Joseph Pecoraro]

Cool idea! Might be good to start out with a Context Menu for each action, and then see which feels better / more useful and make that a default click action.

Created attachment 442778 [details]
Patch

Comment on attachment 442778 [details]
Patch

Various test cases failed when running run-webkit-tests, however, these seem to happen even when I stash my changes. 

Additionally I didn't write a test case for this as per the wiki page below Web Inspector UI changes don't require it. https://trac.webkit.org/wiki/WebInspectorTests

[Patrick Angle]

Comment on attachment 442778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442778&action=review

Thanks for the patch! This is going to be really handy for quickly getting to certificate info. I've left a few comments, but overall this feels like a good approach.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:719
> +                let security = entry.resource.security;
> +                if (isEmptyObject(security)) {
> +                    cell.append(domain || emDash);
> +                    return;
> +                }
> +
> +                let certificate = security.certificate;
> +                if (isEmptyObject(certificate) || Object.values(certificate).every((value) => !value)) {
> +                    cell.append(domain || emDash);
> +                    return;
> +                }
> +
> +                lockIconElement.addEventListener("click", (event) => {
> +
> +                    if (WI.NetworkManager.supportsShowCertificate()) {
> +                        entry.resource.showCertificate()
> +                        .catch((error) => {
> +                            console.error(error);
> +                        });
> +                    }
> +                });

While this works, it would be great to not duplicate the `cell.append(domain || emDash)` in multiple places. Instead of early returns here, we could instead do something along the lines of...
```
let certificate = entry.resource.security?.certificate
if (WI.NetworkManager.supportsShowCertificate() && certificate && Object.values(certificate).some((value) => value)) {
    lockIconElement.addEventListener("click", (event) => {
        // Do nothing with thrown exceptions since `showCertificate` will log any thrown exception.
        entry.resource.showCertificate().catch((error) => {});
    });
}
```
Other considerations here are that we should make sure that an appropriate accessibility label is added to the clickable element (see usage of WI.UIString for examples of how we construct localizable strings like this with three parameters: the text, a key, and details for localizers), and we also probably want to add a bit of styling for the cursor to change to a pointer (on macOS it's the hand with index finger extended cursor) for the element similar to how it changes when you hover over the resource name to the left to indicate that clicking will perform an action.

> WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:-6
> -	<key>BuildSystemType</key>
> -	<string>Original</string>

Probably unintentional; please remove this change from the patch.

Created attachment 443177 [details]
Patch

Created attachment 443180 [details]
Patch

Had some issues with trying to edit the change log but the new patch incorporates Patrick's changes:

1. Reduce cell.append function calls
2. Adjust CSS to change cursor when hovering
3. Integrate (existing) localized UI string to cell.title

Some other things I noticed: the lock icon in the URL bar in safari is labeled as "Show certificate", but the button (and now this icon) in the network details tab use "Show full certificate".

Additionally I noticed that the icons' css aren't differentiated by whether or not the certificate exists, so in certain circumstances the cursor would transform even though the button event doesn't exist (in these cases if you go into the details tab you won't see the "Show full certificate" button either). The only case I've seen this is on the initial webkit.org website in Minibrowser though, so not sure how likely this will show up in production?

[Devin Rousso]

Comment on attachment 443180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443180&action=review

Nice patch!  Some minor comments and then I think we're good to go :)

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:124
> +    cursor: pointer;

We don't normally make clickable icons into a pointer.  Is there a reason why we should in this case?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:699
> +                let security = entry.resource.security;

I'm not sure we can assume that `entry.resource` is valid, so you may need to `entry.resource?.security || {}` instead.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:703
> +                if (!isEmptyObject(security)
> +                    && !isEmptyObject(security.certificate)
> +                    && !Object.values(security.certificate).every((value) => !value)) {

I think you can simplify this
```
    if (WI.NetworkManager.supportsShowCertificate() && Object.values(security?.certificate || {}).some((value) => value))
```

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:704
> +

Style: unnecessary whitespace

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:706
> +

ditto (:704)

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:707
> +                        if (WI.NetworkManager.supportsShowCertificate()) {

please move this to the outer `if` (see comment above) as we don't event want to add the `"click"` event listener if we can't do something with it

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:711
> +                            entry.resource.showCertificate()
> +                            .catch((error) => {
> +                                console.error(error);
> +                            });

you can simplify this to just be `entry.resource.showCertificate().catch(WI.reportInternalError)