[webkit-reviews] review denied: [Bug 191447] Web Inspector: Network: show secure certificate details per-request : [Attachment 354360] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 191447: Web Inspector: Network: show secure certificate details per-request
https://bugs.webkit.org/show_bug.cgi?id=191447

Attachment 354360: Patch

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




--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 354360
  --> https://bugs.webkit.org/attachment.cgi?id=354360
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.