220815 - Non localisable strings in nsCertPicker.cpp and nsNSSIOLayer.cpp

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Jean-Marc Desperrier]

The strings  NICKNAME_EXPIRED_STRING and  NICKNAME_NOT_YET_VALID_STRING in
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsCertPicker.cpp#56
can not be localised, because they are defined with a #define.

 55 /* strings for marking invalid user cert nicknames */
 56 #define NICKNAME_EXPIRED_STRING " (expired)"
 57 #define NICKNAME_NOT_YET_VALID_STRING " (not yet valid)"

Proper code should be used instead so that local packages can localize them.

There's exactly the same code inside nsNSSIOLayer.cpp :
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSIOLayer.cpp#2053
and the same fix should be applied.

Comment 1

[Jean-Marc Desperrier]

I might try to write the patch myself, I take the bug. 
I keep [e-mail] as a CC.

I don't have time to complete it now, but I'll note here how it should be done
as far as I understand from similar code in nsNSSCertificateDB.cpp :

----- In nsCertPicker.cpp, nsNSSIOLayer.cpp

#include "nsNSSComponent.h"

static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);

 nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
 if (NS_FAILED(rv)) return rv;

char *nickExpFmt=NULL;
char *nickNvalFmt=NULL;
nsAutoString tmpNickExpFmt;
nsAutoString tmpNickNvalFmt;

nssComponent->GetPIPNSSBundleString(NS_LITERAL_STRING("NicknameExpired").get(),
tmpNickExpFmt);
nssComponent->GetPIPNSSBundleString(NS_LITERAL_STRING("NicknameNotYetValid").get(),
tmpNickNvalFmt);
nickExpFmt = ToNewUTF8String(tmpNickExpFmt);
nickNvalFmt = ToNewUTF8String(tmpNickNvalFmt);
CERTCertNicknames *nicknames = 
     CERT_NicknameStringsFromCertList(certList, nickExpFmt, nickNvalFmt);

// Apparently, no need to free tmpNickExpFmt or nickExpFmt, at least nothing 
// is done inside nsNSSCertificateDB.cpp

----- certvfy.c :
CERT_ExtractNicknameString :
Ought to be changed to allow nickExpFmt, nickNvalFmt to include a "%s".

Unfortunately, this is a closed interface and I don't believe such a change will
get approval.

----- In pipnss.properties
NicknameExpired= (expired)
NicknameNotYetValid= (not yet valid)

// It would better to do :
// NicknameExpired=%s (expired)
// NicknameNotYetValid=%s (not yet valid)
// but NSS's CERT_ExtractNicknameString inside certvfy.c *assumes* this two 
// strings are concatenated at the end of the name.
// What's more other products using CERT_NicknameStringsFromCertList in NSS 
// could not like the change

Comment 2

[Jean-Marc Desperrier]

Correcting. Thought I was assigning to me, and assigned to ssaux instead.

Comment 3

[Kai Engert (:KaiE:)]

*** Bug 261822 has been marked as a duplicate of this bug. ***

Comment 4

[Jean-Marc Desperrier]

Kaie, now that your working again on crypto, are you willing to take back that bug ?

You're probably able to fix it in minutes.

Comment 5

[Vidar Haarr (not reading bugmail)]

I think the best way to fix this would just be to remove the |char *expiredString| and |char *notYetGoodString| arguments from CERT_NicknameStringsFromCertList, CERT_GetCertNicknameWithValidity and maybe CERT_ExtractNicknameString, and just get the localized data directly here:
<http://lxr.mozilla.org/mozilla/source/security/nss/lib/certhigh/certvfy.c#1856>

Since embedders should be able to override the properties file anyway, they shouldn't need to pass the strings through the API.

Comment 6

[Jean-Marc Desperrier]

(In reply to comment #5)
> I think the best way to fix this would just be to remove the |char
> *expiredString| and |char *notYetGoodString| arguments from
> CERT_NicknameStringsFromCertList, CERT_GetCertNicknameWithValidity 
[..]
> Since embedders should be able to override the properties file anyway, they
> shouldn't need to pass the strings through the API.

Nope. What you miss is that this is a different product : NSS, and NSS is not just a component of the mozilla platform. 
NSS is used in many products ouside Mozilla and we cannot change it's interface just for mozilla. All products that use NSS have different localisation mechanisms, so it must go through the API.

Comment 7

[Vidar Haarr (not reading bugmail)]

Ah, right.

AFAICS, there is no API contract for the following, so we could change the fallback for NULL. Currently if expiredString or notYetGoodString is null, they are assigned "" -- we could make it default to a localizable string from the properties file and then change all internal consumers to just pass null?

Comment 8

[Kai Engert (:KaiE:)]

I'm working on another patch, and I have fixed this in my tree. It will cause conflicts for me if I land it separately now. 

The right solution is to move it into a stringbundle, as this is done with any other strings currently used.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 203941 [details] [diff] [review]
Patch v2

Why are the spaces required at the beginning of those strings, and not put in place
by whatever string-consumer requires it?  That seems like a localization trap waiting
to be sprung.

>+  nssComponent->GetPIPNSSBundleString("NicknameExpired", expiredString);
>+  nssComponent->GetPIPNSSBundleString("NicknameNotYetValid", notYetValidString);

Do we really want to use an empty string for these errors, if we can't
get the string?  Seems like falling back to the English strings would make that
much easier to diagnose (both the nickname error and the fact that the bundle
is broken).

>+  nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsAutoString expiredString, notYetValidString;
>+
>+  nssComponent->GetPIPNSSBundleString("NicknameExpired", expiredString);
>+  nssComponent->GetPIPNSSBundleString("NicknameNotYetValid", notYetValidString);
>+
>+  NS_ConvertUCS2toUTF8 aUtf8ExpiredString(expiredString);
>+  NS_ConvertUCS2toUTF8 aUtf8NotYetValidString(notYetValidString);
>+
>   CERTCertNicknames* nicknames =
>-    CERT_NicknameStringsFromCertList(certList, NICKNAME_EXPIRED_STRING,
>-                                     NICKNAME_NOT_YET_VALID_STRING);
>+    CERT_NicknameStringsFromCertList(certList, 
>+                                     NS_CONST_CAST(char*, aUtf8ExpiredString.get()),
>+                                     NS_CONST_CAST(char*, 

This code is identical to the other copy of it -- is there a reason not to use
a utility function to reduce code size and ease maintenance?

sr=shaver with those items remedied, but feel free to cc: me if you want to explain
them away instead and I'll reconsider.

Comment 12

[Kai Engert (:KaiE:)]

> Why are the spaces required at the beginning of those strings, and not put in
> place by whatever string-consumer requires it?  That seems like a localization > trap waiting to be sprung.

You are right, I have added code that dynamically constructs a string out of a space character and the bundle string appended.


> Do we really want to use an empty string for these errors, if we can't
> get the string?  Seems like falling back to the English strings would make 
> that much easier to diagnose (both the nickname error and the fact that the
> bundle is broken).

It's not an error message. It's part of an information string shown to the user. We don't have the "fall back to english" for most strings. I think this string is not critical enough to justify a fallback logic. (If you think a fallback to english makes sense, shouldn't this be done in the general string bundle code?)


> This code is identical to the other copy of it -- is there a reason not to use
> a utility function to reduce code size and ease maintenance?

Lazyness was the reason to just duplicate the code. But I agree with you, I have now changed that to use a new shared function.


Thanks for the review, I'll attach the patch that has the requested changes and will check it in.

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v3b

Actually the file that now contains the helper function already has a definition for NS_DEFINE_CID so I could get rid of one more line.

Comment 15

[Kai Engert (:KaiE:)]

fixed on trunk

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This patch seems to have put Pacifica in flames

Comment 17

[Kai Engert (:KaiE:)]

no, bug 259031 caused the burning