Bug 191447 – Web Inspector: Network: show secure certificate details per-request

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/30019476>

[Devin Rousso]

Created attachment 354296 [details]
[Patch] WIP

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

[Devin Rousso]

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

[EWS Watchlist]

Attachment 354296 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:344:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:345:  time_1970_01_01 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 354317 [details]
[Patch] WIP

[EWS Watchlist]

Attachment 354317 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CertificateInfoBase.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 354318 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 354360 [details]
Patch

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

[Joseph Pecoraro]

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.

[Devin Rousso]

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 😛

[Devin Rousso]

Created attachment 354412 [details]
Patch

[Devin Rousso]

Created attachment 354418 [details]
Patch

[EWS Watchlist]

Comment on attachment 354418 [details]
Patch

Attachment 354418 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9932310

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354423 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354418 [details]
Patch

Attachment 354418 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9932481

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354428 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354418 [details]
Patch

Attachment 354418 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9933183

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354431 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 354435 [details]
Patch

Fix win build

[Devin Rousso]

Created attachment 354437 [details]
Patch

[Devin Rousso]

Created attachment 354439 [details]
Patch

[Devin Rousso]

Created attachment 354445 [details]
Patch

[Devin Rousso]

Created attachment 354447 [details]
Patch

Add dark mode styles

[Devin Rousso]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Devin Rousso]

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

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 354622 [details]
Patch

[EWS Watchlist]

Attachment 354622 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 354635 [details]
Patch

Fix debug build

[EWS Watchlist]

Attachment 354635 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 354635 [details]
Patch

Attachment 354635 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9968339

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 354647 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 354635 [details]
Patch

Clearing flags on attachment: 354635

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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