| Summary: | Web Inspector: Display Content Security Policy hash in details sidebar for script and style elements | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
| Component: | Web Inspector | Assignee: | Daniel Bates <dbates> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bburg, bfulgham, commit-queue, graouts, joepeck, keith_miller, mark.lam, mattbaker, mkwst, msaboff, nvasilyev, saam, timothy, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Daniel Bates
2016-03-14 14:35:44 PDT
Created attachment 274031 [details]
Patch and Layout Tests
Created attachment 274032 [details]
[Screenshot] Example UI
A screenshot that illustrates the appearance of a CSP hash in the sidebar when an HTML script element (for an inline JavaScript script) is selected.
Comment on attachment 274031 [details] Patch and Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=274031&action=review r=me! Awesome. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1284 > + return makeString("'sha256-", base64Encode(digest.data(), digest.size()), '\''); Are these single-quotes necessary? > Source/WebCore/inspector/InspectorFrontendHost.h:41 > +class Element; Is this needed? I don't see it used. > LayoutTests/inspector/dom/csp-hash.html:1 > +<html> We normally add a DOCTYPE. Would that affect things here? > LayoutTests/inspector/dom/csp-hash.html:48 > + WebInspector.domTreeManager.querySelector(documentNode.id, "#script-with-uncode-code-point-00C5", function(nodeId) { Typo: "uncode" => "unicode". Here and a few other places. > LayoutTests/inspector/dom/csp-hash.html:62 > + WebInspector.domTreeManager.querySelector(documentNode.id, "#script-with-uncode-code-point-212B", function(nodeId) { > + let domNode = WebInspector.domTreeManager.nodeForId(nodeId); > + let expectedHash = "'sha256-YcKgriaBGkU6FsWZXgDLv4Wo5UZ5Qe5hNp6Psb3RJOE='"; // Same hash as for script #script-with-uncode-code-point-00C5. > + InspectorTest.log(""); > + InspectorTest.expectThat(domNode, "Got DOMNode for #script-with-uncode-code-point-212B"); > + InspectorTest.expectThat(domNode.contentSecurityPolicyHash() === expectedHash, `DOMNode has hash ${expectedHash}`); > + }); An easier way to write this file could be a set of test that you loop through. Since it seems the contents of all of these functions are nearly identical. // Nit: Style this however you wish. let testCases = [ {selector: '#stylesheet-without-whitespace", hash: "'sha256-NW7+Fm6YV404pkklaopT0jgCBCmfOAn0K+NtIfyPN4A='"}, {selector: '#stylesheet-with-whitespace", hash: "'sha256-NW7+Fm6YV404pkklaopT0jgCBCmfOAn0K+NtIfyPN4A='"}, ... ]; for (let {selector, hash} of testCases) { WebInspector.domTreeManager.querySelector(documentNode.id, test.selector, function(nodeId) { let domNode = WebInspector.domTreeManager.nodeForId(nodeId); InspectorTest.log(""); InspectorTest.expectThat(domNode, `Got DOMNode for ${selector}`); InspectorTest.expectThat(domNode.contentSecurityPolicyHash() === hash, `DOMNode has hash ${hash}`); } } InspectorTest.completeTest(); > LayoutTests/inspector/dom/csp-hash.html:90 > + <p>Test for Content Security Policy hash support.</p> Please add " on DOM.DOMNode". > LayoutTests/inspector/dom/csp-hash.html:113 > + <!-- Hash of this script should be equivalent to hash of script script-with-uncode-code-point-00C5. --> Typo: "uncode" => "unicode" Comment on attachment 274031 [details] Patch and Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=274031&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1284 >> + return makeString("'sha256-", base64Encode(digest.data(), digest.size()), '\''); > > Are these single-quotes necessary? They are needed when putting the hash in the CSP header, but they clutter the UI. Lets drop them. A developer will know to add them if they use this. > Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:41 > + this._identityNodeContentSecurityPolicyHashRow = new WebInspector.DetailsSectionSimpleRow(WebInspector.UIString("Content Security Policy Hash")); This is a long label. I think we can just say "CSP Hash", most developers will know what that is if they need it. (In reply to comment #4) > > Source/WebCore/inspector/InspectorDOMAgent.cpp:1284 > > + return makeString("'sha256-", base64Encode(digest.data(), digest.size()), '\''); > > Are these single-quotes necessary? > As mentioned in Timothy Hatcher's remark in comment #5, the single quotes are needed "when putting the hash in the CSP header". Timothy Hatcher suggested in the same comment that we omit these single quotes. > > Source/WebCore/inspector/InspectorFrontendHost.h:41 > > +class Element; > > Is this needed? I don't see it used. > No, it is not needed. Will remove. > > LayoutTests/inspector/dom/csp-hash.html:1 > > +<html> > > We normally add a DOCTYPE. Will add DOCTYPE tag. > Would that affect things here? > No. > > LayoutTests/inspector/dom/csp-hash.html:48 > > + WebInspector.domTreeManager.querySelector(documentNode.id, "#script-with-uncode-code-point-00C5", function(nodeId) { > > Typo: "uncode" => "unicode". Here and a few other places. > Will fix throughout this file. > > LayoutTests/inspector/dom/csp-hash.html:62 > > + WebInspector.domTreeManager.querySelector(documentNode.id, "#script-with-uncode-code-point-212B", function(nodeId) { > > + let domNode = WebInspector.domTreeManager.nodeForId(nodeId); > > + let expectedHash = "'sha256-YcKgriaBGkU6FsWZXgDLv4Wo5UZ5Qe5hNp6Psb3RJOE='"; // Same hash as for script #script-with-uncode-code-point-00C5. > > + InspectorTest.log(""); > > + InspectorTest.expectThat(domNode, "Got DOMNode for #script-with-uncode-code-point-212B"); > > + InspectorTest.expectThat(domNode.contentSecurityPolicyHash() === expectedHash, `DOMNode has hash ${expectedHash}`); > > + }); > > An easier way to write this file could be a set of test that you loop > through. Since it seems the contents of all of these functions are nearly > identical. > > // Nit: Style this however you wish. > let testCases = [ > {selector: '#stylesheet-without-whitespace", hash: > "'sha256-NW7+Fm6YV404pkklaopT0jgCBCmfOAn0K+NtIfyPN4A='"}, > {selector: '#stylesheet-with-whitespace", hash: > "'sha256-NW7+Fm6YV404pkklaopT0jgCBCmfOAn0K+NtIfyPN4A='"}, > ... > ]; > > for (let {selector, hash} of testCases) { > WebInspector.domTreeManager.querySelector(documentNode.id, > test.selector, function(nodeId) { > let domNode = WebInspector.domTreeManager.nodeForId(nodeId); > InspectorTest.log(""); > InspectorTest.expectThat(domNode, `Got DOMNode for ${selector}`); > InspectorTest.expectThat(domNode.contentSecurityPolicyHash() === > hash, `DOMNode has hash ${hash}`); > } > } > > InspectorTest.completeTest(); > Will use your idea to simplify the code in this file (cap-hash.html) and LayoutTests/inspector/dom/csp-big5-hash.html. > > LayoutTests/inspector/dom/csp-hash.html:90 > > + <p>Test for Content Security Policy hash support.</p> > > Please add " on DOM.DOMNode". > Will add. > > LayoutTests/inspector/dom/csp-hash.html:113 > > + <!-- Hash of this script should be equivalent to hash of script script-with-uncode-code-point-00C5. --> > > Typo: "uncode" => "unicode" Will fix throughout this file. (In reply to comment #5) > >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1284 > >> + return makeString("'sha256-", base64Encode(digest.data(), digest.size()), '\''); > > > > Are these single-quotes necessary? > > They are needed when putting the hash in the CSP header, but they clutter > the UI. Lets drop them. A developer will know to add them if they use this. > Will omit the single quotes. > > Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:41 > > + this._identityNodeContentSecurityPolicyHashRow = new WebInspector.DetailsSectionSimpleRow(WebInspector.UIString("Content Security Policy Hash")); > > This is a long label. I think we can just say "CSP Hash", most developers > will know what that is if they need it. Will change label to read "CSP Hash". Committed r198178: <http://trac.webkit.org/changeset/198178> |