946075 - ssl_error_bad_cert_domain error message contains extra space: " , " instead of ", "

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

For example, at the time I filed this bug, https://ietf.org was giving a ssl_error_bad_cert_domain error, and the "Technical details" message contained this:

"The certificate is only valid for the following names: *.irtf.org , irtf.org"

We should s/ , /, /

This bug is probably in AppendErrorTextMismatch in security/manager/ssl/src/TransportSecurityInfo.cpp

Comment 1

[Nemina Amarasinghe]

Hi Brian,

I would like to work on this bug. Please assign me.. :)

Thank you
Nemina

Comment 2

[Nemina Amarasinghe]

Attachment: Removed the extra space as reqested in the bug

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8356530 [details] [diff] [review]
Removed the extra space as reqested in the bug

Review of attachment 8356530 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, Nemina!
A couple of comments:
- in the commit message, the convention is to have a space after "bug"
- also, something like "removed extra space in cert domain mismatch message" might be a bit more descriptive
- somehow your change on line 708 didn't get properly picked up in the patch - you could try regenerating the patch, making sure you're using a recent checkout ('hg pull && hg update', etc.)

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +704,5 @@
>      switch (current->type) {
>        case certDNSName:
>          name.AssignASCII((char*)current->name.other.data, current->name.other.len);
>          if (!allNames.IsEmpty()) {
> +          allNames.Append(NS_LITERAL_STRING(" , "));

Something strange happened on this line - this is what the code currently is, not what it should be changed to (which is what this patch says it is changing it from).

@@ +729,5 @@
>              /* invalid IP address */
>            }
>            if (!name.IsEmpty()) {
>              if (!allNames.IsEmpty()) {
> +              allNames.Append(NS_LITERAL_STRING(", "));

This is correct.

Comment 4

[Nemina Amarasinghe]

Thanx for the comments brian!.. 
Last few weeks I had really bad time trying to building Firefox... it was fun though . I will do my best to get this right.
Thanks again

Comment 5

[Nemina Amarasinghe]

Attachment: Removed the extra space in SSL error, bad certifacte message

Comment 6

[Nemina Amarasinghe]

(In reply to [e-mail] from comment #4)
> Thanx for the comments brian!.. 
> Last few weeks I had really bad time trying to building Firefox... it was
> fun though . I will do my best to get this right.
> Thanks again

Sorry for the miss David, I miss addressed you as Brian.. :(

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8357067 [details] [diff] [review]
Removed the extra space in SSL error, bad certifacte message

Review of attachment 8357067 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me
No worries about mistaking me for Brian :)
If you're not familiar with the check-in process, I would have a look at this: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed (it looks like you've already done most of that, but for instance I would put your email address after your name as the author of the patch).

Comment 8

[Nemina Amarasinghe]

Thanx for the blog David, I searched lot about these things but still I don't have a clear idea about patches n stuff.. If you have other reading or video    material about these things please give some links. :) ..
Really appreciate the help that the mozilla dev group give.... 
Thanx again David...

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Here's some good documentation:
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Mercurial
https://developer.mozilla.org/en-US/docs/Developer_Guide

I think all you need to do at the moment is:
- make sure the tree builds with your patch if you haven't already (it would be a good idea to double-check that it does what you expect, too)
- update your username in your .hgrc to be something like "Nemina Amarasinghe <[e-mail]>" (so your email gets included in patches you generate)
- upload a new patch with the changed username (you can set the r+ flag yourself and say something like "carrying over r+" in the bugzilla comment) and an updated commit message that includes "r=keeler"
- put "checkin-needed" to the keywords field of this bug

If any of this doesn't work, you might need some additional permissions added to your bugzilla account - I can get someone to do that for you.
Also, you might check out IRC: https://wiki.mozilla.org/IRC (I'm "keeler" and I hang out in #security)

Comment 10

[Nemina Amarasinghe]

Thanks David... I'm new to this hg things.. I definitely try this things today...:D (y)

Comment 11

[Nemina Amarasinghe]

This patch is ready to be checked in, can someone help me.. :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64a08871e038

Thanks for the patch! This is now landed on a staging branch. Eventually, it will be merged to mozilla-central and the bug will be closed. One request, please make sure that your Mercurial commit information includes your email address in addition to your name. Thanks :)

Comment 13

[Nemina Amarasinghe]

Thanx Ryan I'll add my email as you said :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64a08871e038