<rdar://problem/30019476>

Created attachment 354296 [details]
[Patch] WIP

Testing to see how this builds on various platforms.  Will add tests after EWS.

Created attachment 354297 [details]
[Image] After Patch is applied

Created attachment 354317 [details]
[Patch] WIP

Created attachment 354318 [details]
[Patch] WIP

Created attachment 354360 [details]
Patch

Split out code for <https://webkit.org/b/191458>

Comment on attachment 354360 [details]
Patch

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

Overall this looks good to me. I'll just want to see an updated patch.

> Source/JavaScriptCore/inspector/protocol/Security.json:8
> +            "description": "Security information for the request.",

This description is poor.

> Source/JavaScriptCore/inspector/protocol/Security.json:11
> +                { "name": "created", "type": "number", "optional": true },

Is this really a `created` date or the valid start date?
Given these are seconds since epoch, shouldn't these be $ref of "Network.Walltime"?

> Source/JavaScriptCore/inspector/protocol/Security.json:14
> +                { "name": "ipAddresses", "type": "array", "items": { "type": "string" }, "optional": true }

Could this get a description. What are these ip addresses? Specific hosts the certificate is limited to?

> Source/WebCore/loader/ResourceLoader.h:130
> +    WEBCORE_EXPORT bool shouldIncludeCertificateInfo() const;

Was the WEBCORE_EXPORT needed? I only see uses in WebCore.

> Source/WebCore/platform/network/CertificateInfoBase.h:37
> +    bool containsNonRootSHA1SignedCertificate() const { return false; }

Should all of these methods be virtual? Otherwise, how do these call down into the sub-class implementations?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:108
> +    auto firstCertificate = checked_cf_cast<SecCertificateRef>(CFArrayGetValueAtIndex(chain, 0));

Perhaps we should name this `leafCertificate`.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.css:31
> +    color: var(--network-dns-color);

How does this look in dark mode? Is it readable?

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:66
> +        let hasInsecureScheme = this._resource.urlComponents.scheme !== "https" && this._resource.urlComponents.scheme !== "wss" && this._resource.urlComponents.scheme !== "sftp";

This seems like it could be a helper somewhere. If generalized I wonder if we should also `data:` as secure even if it shouldn't be reached here.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:72
> +        if (hasInsecureConnectionTiming || hasInsecureScheme) {
> +            if (!this._insecureMessageElement)
> +                this._insecureMessageElement = WI.createMessageTextView(WI.UIString("The resource was requested insecurely."), true);
> +            this.element.appendChild(this._insecureMessageElement);
> +            return;
> +        }

Can we get a screenshot of this appearance?

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:178
> +            this._certificateSection.markIncompleteSectionWithMessage(WI.UIString("No response security information"));

Normally the incomplete section messages end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:184
> +            this._certificateSection.markIncompleteSectionWithMessage(WI.UIString("No response security certificate"));

Normally the incomplete section messages end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:190
> +        let formatDate = (key, timestamp) => {

Maybe name this `appendFormattedDate`.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:203
> +        formatDate(WI.UIString("Created"), certificate.created);
> +        formatDate(WI.UIString("Expires"), certificate.expires);

I still question if this is "Created".

  - Apple's Native UI: "Not Valid Before", "Not Valid After"
  - Chrome: "Valid From", "Valid Until"
  - Firefox: "Begins On", "Expires On"

We could match the Apple Native UI, but I tend to prefer wording like these:

  - "Valid From", "Valid Until"
  - "Valid Starting", "Valid Ending"
  - "Valid Beginning", "Expires"

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:205
> +        let limitValues = (key, values, className) => {

Maybe name this `appendList`

> LayoutTests/ChangeLog:10
> +        * http/tests/inspector/network/resource-response-security-expected.txt: Added.
> +        * http/tests/inspector/network/resource-response-security.html: Added.

You may need to skip this on other ports, I don't think they will have dates for example.

Comment on attachment 354360 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoader.h:130
>> +    WEBCORE_EXPORT bool shouldIncludeCertificateInfo() const;
> 
> Was the WEBCORE_EXPORT needed? I only see uses in WebCore.

It's used once in WebKit/WebProcess/Network/WebLoaderStrategy.cpp:274

>> Source/WebCore/platform/network/CertificateInfoBase.h:37
>> +    bool containsNonRootSHA1SignedCertificate() const { return false; }
> 
> Should all of these methods be virtual? Otherwise, how do these call down into the sub-class implementations?

The sub-classes fully override these methods where needed.  Then again, there's no real reason for me to not make them virtual.

>> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:108
>> +    auto firstCertificate = checked_cf_cast<SecCertificateRef>(CFArrayGetValueAtIndex(chain, 0));
> 
> Perhaps we should name this `leafCertificate`.

I think that's the actual name for it 😛

Created attachment 354412 [details]
Patch

Created attachment 354418 [details]
Patch

Created attachment 354435 [details]
Patch

Fix win build

Created attachment 354437 [details]
Patch

Created attachment 354439 [details]
Patch

Created attachment 354445 [details]
Patch

Created attachment 354447 [details]
Patch

Add dark mode styles

Created attachment 354448 [details]
[Image] After Patch is applied (Insecure)

(In reply to Devin Rousso from comment #24)
> Created attachment 354448 [details]
> [Image] After Patch is applied (Insecure)

Hmm, this doesn't look particularly good. I'd expect normal sectioning with it saying insecure not an entire content view. For now this is good but we should improve on this.

Comment on attachment 354447 [details]
Patch

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

r=me with comments

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
> +/*

Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:123
> +        const Seconds absoluteReferenceDate(978307200);

Probably deserves a comment. This is the 1970 epoch?

> Source/WebCore/platform/network/cf/CertificateInfo.h:-76
> -#ifndef NDEBUG
> -    void dump() const;
> -#endif

You could still provide a `dump()` implementation in a platform/network/cocoa/CertificateInfoCocoa.mm. Here is would be, and the implementation would be identical to what it was before.

    #ifndef NDEBUG
    #if PLATFORM(COCOA)
        void dump() const;
    #endif
    #endif

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:65
> +        if (!this._resource.loadedSecurely) {

What if we don't have any network details at this point.

Scenario:
1. Load webkit.org
2. Open inspector
3. Go to Network Tab
4. Select a Resource
5. Select Security
  => We don't have secure timing details, does this mean it says loaded insecurely!?

We might want to present the "incomplete details" text.

Comment on attachment 354447 [details]
Patch

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

>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
>> +/*
> 
> Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.

None of the other platform specific `CertificateInfo` files had their respective platform as a suffix, so I was matching how they did it.  I'll change all of them to match their respective platforms.

>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:123
>> +        const Seconds absoluteReferenceDate(978307200);
> 
> Probably deserves a comment. This is the 1970 epoch?

No, this is 1 Jan 2001 00:00:00 GMT
<https://developer.apple.com/documentation/corefoundation/cfabsolutetime>

>> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:65
>> +        if (!this._resource.loadedSecurely) {
> 
> What if we don't have any network details at this point.
> 
> Scenario:
> 1. Load webkit.org
> 2. Open inspector
> 3. Go to Network Tab
> 4. Select a Resource
> 5. Select Security
>   => We don't have secure timing details, does this mean it says loaded insecurely!?
> 
> We might want to present the "incomplete details" text.

`loadedSecurely` only returns false if:
  1. the URL scheme is not "https", "wss", or "sftp"
  2. there IS a `connectionStart` but there ISN'T a `secureConnectionStart` (e.g. the timing started a connection but never started a secure connection)

In the case that WebInspector is opened after the resource has finished loading, `loadedSecurely` will only use (1), as there won't be a `connectionStart` so (2) won't apply

Comment on attachment 354447 [details]
Patch

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

>>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
>>> +/*
>> 
>> Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.
> 
> None of the other platform specific `CertificateInfo` files had their respective platform as a suffix, so I was matching how they did it.  I'll change all of them to match their respective platforms.

Actually, I think this is so that including "CertificateInfo.h" will work regardless of platform (see ResourceResponse for another example).  I'll only change the *.cpp files.

Created attachment 354622 [details]
Patch

Created attachment 354635 [details]
Patch

Fix debug build

Comment on attachment 354635 [details]
Patch

Clearing flags on attachment: 354635

Committed r238122: <https://trac.webkit.org/changeset/238122>

All reviewed patches have been landed.  Closing bug.

It looks like revision https://trac.webkit.org/changeset/238122/webkit

has caused multiple crashing tests on Mac Debug WK2

Crashing tests:
inspector/worker/console-basic.html
inspector/worker/debugger-pause.html
inspector/worker/debugger-scripts.html
inspector/worker/runtime-basic.html
inspector/worker/worker-create-and-terminate.html

Log for inspector/worker/console-basic.html:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r238129%20(5514)/inspector/worker/console-basic-crash-log.txt

Combined History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fworker%2Fconsole-basic.html%20inspector%2Fworker%2Fdebugger-pause.html%20inspector%2Fworker%2Fdebugger-scripts.html%20inspector%2Fworker%2Fruntime-basic.html%20inspector%2Fworker%2Fworker-create-and-terminate.html