Closed
Bug 1075081
Opened 7 years ago
Closed 7 years ago
Enhance pinning test to ensure the neterror page is actually the one found
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
Attachments
(1 file, 2 obsolete files)
2.34 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
On landing of 787133 I missed checking that the actual error was the pinning failure and not other. (Tough this check is implied by the subsequent passing with removal of pins).
Blocks: hpkp
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8497720 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8497723 -
Flags: review?(dkeeler)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cviecco
Comment on attachment 8497723 [details] [diff] [review] add-checking-is-pinnning-error Review of attachment 8497723 [details] [diff] [review]: ----------------------------------------------------------------- Looks like there's still some cleanup to do. ::: browser/base/content/test/general/browser_blockHPKP.js @@ +70,3 @@ > // to load the pinning domain again, this time removing the pinning information > let certErrorProgressListener = { > buttonClicked: false, don't need this @@ +71,5 @@ > let certErrorProgressListener = { > buttonClicked: false, > onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { > if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) { > let self = this; probably don't need this @@ +73,5 @@ > onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { > if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) { > let self = this; > // Can't directly call button.click() in onStateChange > executeSoon(function() { we're not calling button.click(), so we probably don't need the executeSoon @@ +79,1 @@ > // If about:neterror hasn't fully loaded, the button won't be present. again, we don't care about the button, but it looks like we care about 'errorShortDescText' @@ +79,3 @@ > // If about:neterror hasn't fully loaded, the button won't be present. > // It will eventually be there, however. > if (button && !self.buttonClicked) { again, this isn't what we're predicating on @@ +81,5 @@ > if (button && !self.buttonClicked) { > + let textElement = content.document.getElementById("errorShortDescText"); > + let text = textElement.innerHTML; > + ok(text.indexOf("mozilla_pkix_error_key_pinning_failure") > 0, > + "Got an pinning error page"); nit: "a pinning..." @@ +86,1 @@ > gBrowser.removeProgressListener(self); without the executeSoon, we can use 'this' instead of 'self'
Attachment #8497723 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8497723 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8498930 -
Flags: review?(dkeeler)
Comment on attachment 8498930 [details] [diff] [review] add-checking-is-pinnning-error (v2) Review of attachment 8498930 [details] [diff] [review]: ----------------------------------------------------------------- Great.
Attachment #8498930 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/944246c0d39f
Comment 7•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/944246c0d39f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•