This string landed in bug 1207146 https://hg.mozilla.org/mozilla-central/diff/de962dcbc06c/browser/locales/en-US/chrome/browser/browser.properties certErrorDetails.label = %1$S\r\n\r\n%2$S\r\n\r\nHTTP Strict Transport Security: %3$S\r\nHTTP Public Key Pinning: %4$S\r\n\r\nCertificate chain:\r\n\r\n%5$S I find two issues with this string: * Some tools don't exactly know how to handle \r\n (see bug 1126171). * It's extremely messy. My first suggestion would be to split it up, since we don't need to let localizers control whitelines. certErrorDetailsHSTS.label = HTTP Strict Transport Security: %S certErrorDetailsKeyPinning.label = HTTP Public Key Pinning: %S certErrorDetailsCertChain.label = Certificate chain: %S And then chain the strings in code with \r\n (also note that we don't need to expose placeholders for URL and error message).

I'd like to work on this bug, how would I go about testing this though?

Just visiting https://wrong.host.badssl.com/ (or other similar test pages in badssl.com) will result in the about:certerror page being displayed. If you build Firefox with the fix applied, you should be able to confirm that opening the advanced section and clicking on the error code displays the debugging information properly.

I'm working on this bug, however I can't find a great way to return at: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3204 At other places in browser.js `getFormattedString()` is being called using an array as the param, but I don't think it can be done as the key (for the labels in browser.properties). So, if we were to do this based on comment #0, how would I pass the labels in browser.properties via getFormattedString properly? In browser.js I tried creating an array with the label names and passing it to the function but it doesn't seem to work. Absolutely any information would be greatly appreciated!

(In reply to Tyler Steiman from comment #3) > I'm working on this bug, however I can't find a great way to return at: > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#3204 > > At other places in browser.js `getFormattedString()` is being called using > an array as the param, but I don't think it can be done as the key (for the > labels in browser.properties). So, if we were to do this based on comment > #0, how would I pass the labels in browser.properties via getFormattedString > properly? In browser.js I tried creating an array with the label names and > passing it to the function but it doesn't seem to work. Absolutely any > information would be greatly appreciated! I think you'd just need to call that function 3 times with the 3 different strings, and join up the results with the '\r\n' separators manually. Does that make sense?

Well the function in browser.js `getDetailedCertErrorInfo(location, securityInfoAsString)` is returning the string in question here. It's not taking the string we're working with as an argument, rather it's calling `getFormattedString()` in the return statement of the function. Because of that I don't know which way to go here.

(In reply to Tyler Steiman from comment #5) > Well the function in browser.js `getDetailedCertErrorInfo(location, > securityInfoAsString)` is returning the string in question here. It's not > taking the string we're working with as an argument, rather it's calling > `getFormattedString()` in the return statement of the function. Because of > that I don't know which way to go here. My point was, rather than doing: return gNavigatorBundle.getFormattedString("certErrorDetails.label", details, 5); you want to be doing: let rv = gNavigatorBundle.getFormattedString("certErrorDetails.HSTS.label", somePartOfDetails, somePartOfDetails.length); rv += "\r\n" + gNavigatorBundle.getFormattedString("certErrorDetails.pinning.label", someOtherThings, someOtherThings.length); rv += "\r\n" + gNavigatorBundle.getFormattedString("certErrorDetails.certchain.label", certChainThingies, certChainThingies.length); You probably want to pick better variable names, and I've left figuring out the right items that you need for each string up to you. But this should work. Does that make more sense?

Yes it absolutely makes sense, sorry for the confusion!!

I've attached what I have so far. The formatting is off though. I'll attach a screenshot of how it's getting formatted. It seems to be like this no matter what I change, including if I change the properties file itself ... not really sure how to fix that.

Screenshot of how it's being formatted.

Comment on attachment 8699765 [details] [diff] [review] bug1231312_simplify_certErrorsString.diff Review of attachment 8699765 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! ::: browser/base/content/browser.js @@ +3200,5 @@ > certChain += getPEMString(cert); > } > } > details.push(certChain); > + let certErrorDetails = gNavigatorBundle.getFormattedString("certErrorDetailsURL.label", [details, details.length]); This is not the right way to specify a single replacement string. See other uses of |getFormattedString| in this file. You keep sending the entire |details| array, which is now redundant. What you could do instead is construct the certErrorDetails string as you gather the information in |getDetailedCertErrorInfo|, rather than keep storing the intermediate strings in an array to use later. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +809,5 @@ > # supports HPKP, %5$S is the certificate chain in PEM format. > + > +#certErrorDetails.label = %1$S\r\n\r\n%2$S\r\n\r\nHTTP Strict Transport Security: %3$S\r\nHTTP Public Key Pinning: %4$S\r\n\r\nCertificate chain:\r\n\r\n%5$S > +certErrorDetailsURL.label = Visited URL: %S > +certErrorDetailsError.label = Error: %S No need to craft new strings for these two, and then you could use flod's advice: (from comment #0) > (also note that we don't need > to expose placeholders for URL and error message).

I hope this is (a little) better. I'll attach a screen below, thanks for the patience I know I'm still a rookie :P. The only thing I couldn't really smooth out was how to separate "Certificate chain:" from the chain itself ... which doesn't look too nice.

Looks good (to me) except for not being able to separate "Certificate chain: "

Comment on attachment 8700089 [details] [diff] [review] bug1231312_simplify_certErrorsString.diff Review of attachment 8700089 [details] [diff] [review]: ----------------------------------------------------------------- This is progress! ::: browser/base/content/browser.js @@ +3166,5 @@ > if (!securityInfoAsString) > return ""; > > + let certErrorDetails = ""; > + certErrorDetails += "\r\n\r\n" + location; This can be replaced with the more succinct: let certErrorDetails = location; We don't need extra whitespace before the first string. @@ +3189,5 @@ > > let uri = Services.io.newURI(location, null, null); > + > + certErrorDetails += "\r\n\r\n" + gNavigatorBundle.getFormattedString("certErrorDetailsHSTS.label", > + [sss.isSecureHost(sss.HEADER_HSTS, uri.host, flags), sss.isSecureHost(sss.HEADER_HSTS, uri.host, flags).length]); You don't need to add a length in any of these cases: just something like [foo] will suffice. Did you check the other instances of getFormattedString() in this file as I suggested? @@ +3190,5 @@ > let uri = Services.io.newURI(location, null, null); > + > + certErrorDetails += "\r\n\r\n" + gNavigatorBundle.getFormattedString("certErrorDetailsHSTS.label", > + [sss.isSecureHost(sss.HEADER_HSTS, uri.host, flags), sss.isSecureHost(sss.HEADER_HSTS, uri.host, flags).length]); > + certErrorDetails += "\r\n\r\n" + gNavigatorBundle.getFormattedString("certErrorDetailsKeyPinning.label", The current code has a single pair of \r\n between HSTS and HPKP. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +800,5 @@ > # LOCALIZATION NOTE (weakCryptoOverriding.message): %S is brandShortName > weakCryptoOverriding.message = %S recommends that you don't enter your password, credit card and other personal information on this website. > revokeOverride.label = Don't Trust This Website > revokeOverride.accesskey = D > +# LOCALIZATION NOTE (certErrorDetails.label): These are text strings that You now have to change the ID to something like "(certErrorDetails*.label)". You should also remove the empty line right after the comment to indicate that it applies to the lines that follow. @@ +806,5 @@ > +# the server administrators for troubleshooting. > + > +certErrorDetailsHSTS.label = HTTP Strict Transport Security: %S > +certErrorDetailsKeyPinning.label = HTTP Public Key Pinning: %S > +certErrorDetailsCertChain.label = Certificate chain: %S If you look closely at the string you are replacing, you will find there are newlines between "Certificate chain:" and the actual certificate chain string. What you need to do here is to not use a substitution string in certErrorDetailsCertChain.label and just append the newlines and the string after it. You should use gNavigatorBundle.getString() in this case.

>What you need to do here is to not use a substitution string in >certErrorDetailsCertChain.label and just append the newlines and >the string after it. You should use gNavigatorBundle.getString() in this case. Does this mean `certErrorDetailsCertChain.label` should only be %S and accessed by browser.js with `gNavigatorBundle.getString()`, being appended to the new lines?

(In reply to Tyler Steiman from comment #14) > >What you need to do here is to not use a substitution string in > >certErrorDetailsCertChain.label and just append the newlines and > >the string after it. You should use gNavigatorBundle.getString() in this case. > > Does this mean `certErrorDetailsCertChain.label` should only be %S and > accessed by browser.js with `gNavigatorBundle.getString()`, being appended > to the new lines? No, it's actually the opposite: certErrorDetailsCertChain.label = Certificate chain: And then you use getString for this.

Would something like this work? Sorry for the delay, I've been gone for the holidays.

(In reply to Tyler Steiman from comment #16) > Would something like this work? Sorry for the delay, I've been gone for the > holidays. No worries, you only neglected one piece of feedback I gave earlier: (In reply to Panos Astithas [:past] from comment #13) > ::: browser/locales/en-US/chrome/browser/browser.properties > > +# LOCALIZATION NOTE (certErrorDetails.label): These are text strings that > > You now have to change the ID to something like "(certErrorDetails*.label)". > You should also remove the empty line right after the comment to indicate > that it applies to the lines that follow. If you fix that, then I can land this.

Sorry for missing that comment!! Here's a patch with those updates.

Comment on attachment 8704309 [details] [diff] [review] bug1231312_simplify_certErrorsString.diff Review of attachment 8704309 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks great!

Pushed after reformatting some overly long lines.

Not necessary, there is no visible change.

Great! Thanks for all the help!

Anytime!

[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected