360015 - pipnss.properties should not use use branded strings

Status: VERIFIED WORKSFORME

(Core :: Security: PSM, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Bug 236933 changed the SSL/weak ciphers strings to be branded strings:

> #for the next 3 lines, %1$S will be product name (e.g. Firefox), %2$S will be the www.example.com address we connect to

This leads to issues in embedding apps, where dialogs read "(null)" instead of the appname because embedding apps don't ship a branding package by default, and making embedding apps do so 1) has frozen API implications (bug 302309 comment 5) and 2) is very difficult to do properly (bug 302309 comment 18 and 23).

bmsedberg says "Unless somebody comes up with a way to do a "default branding package", which I think is a very difficult problem (and not something we can deal with on branches), we should instead avoid using branding strings from embeddable code.  So the NSS issue would be a bug [...]."

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm very aware there are evil l10n implications for changing these strings on the branch (screwing Firefox/Tbird l10n freeze), but right now the strings are nasty to embeddors (like Camino, where our security dialogs have lots of "(null)" where the appname should be in these strings), so I'm requesting 1.8.1.1 for discussion.

Comment 2

[Mike Connor [:mconnor]]

We're not realistically going to do this for 1.8.1.x without coordinating a lot of resources and investing a lot of time in recreating langpacks etc.  It's mildly unpolished, but doesn't really break things for Camino users, IMO.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I should note that if this were happening in Firefox it would be considered a stop-ship bug, given past history.  So I'm not sure why you think this isn't really a problem for Camino...

Unless we're doing the usual double-standard between what MoCo ships and what other MoFo projects ship.

Comment 4

[Mike Connor [:mconnor]]

Two points:

If this was happening in Firefox, we wouldn't have left it until months after string freeze to bring this up, especially since the reporter was aware of this in June, based on bug 236933.  It should have been nominated for blocking then, not long after we have strings frozen on the branch.

Given how rarely we encounter this error (SSL2 and/or no cipher mismatch) I wouldn't block Firefox 2.0.0.1 over this if we had the same bug.  We consciously shipped Firefox 2 for Japan with these dialogs broken (see bug 357050) so I wouldn't call it stop-ship...

Comment 5

[Axel Hecht [:Pike]]

As mconnor said the magic word, does camino use the standard langpacks or is it using new ones?

If they're using special ones, you can hack away the branding by using

%1$.1S

which results in hiding the product name. This is what we did for Japanese for 2.0.0.1.

Comment 6

[Stuart Morgan]

(In reply to comment #5)
> If they're using special ones, you can hack away the branding

Thanks, that does indeed work for Camino as a temporary fix, and I've updated bug 343837 (which tracks Camino's need to hack around this somehow until it's fixed in core) with that information.

I do want to point out that while it should have been nominated for blocking sooner, it's somewhat unfair to say that we waited for months to bring this up. Smokey commented in the bug that caused this *before it landed*, specifically saying that it broke Camino and that the conclusion reached in similar cases was that it wasn't okay for these sorts of strings to rely on branding. So it was brought up, and it's unfortunate that the knowledge that it would break embedders wasn't sufficient to prevent it from landing in that form.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> we wouldn't have left it until months after string freeze to bring this up

It _was_ brought up before freeze.  Possibly not to drivers (which is a problem probably having to do with it being unclear which drivers issues like this should be brought up to... the Firefox ones?), but certainly to the module owner when the patch was being reviewed.  And the issue was ignored, as usual for "not the cool thing we're doing right now" projects.

Comment 8

[Kai Engert [:KaiE:]]

The 3 strings in question all start with
  (Brand) can't ...

Should we just change that to
  Can't ...

?

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Patch for discussion. Comments or reviews welcome.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Who should review this?  I know then l10n freeze for 1.9 is on the horizon....

I think I'd prefer "This application can't", but Kai's "Can't" proposal is certainly workable. 

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 248535 [details] [diff] [review]
Patch v1

Benjamin, you asked me to review my own patch. Not sure if that's helpful, but r=kengert :-)

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 248535 [details] [diff] [review]
Patch v1

Asking Bob for review

Comment 13

[Robert Relyea]

2 questions:

1) should the bundle strings get new names so translators know to retranslate them.
2) what about using the branded string if the brand exists an the non-branded string if it doesn't.

bob

Comment 14

[Kai Engert [:KaiE:]]

(In reply to comment #13)
> 1) should the bundle strings get new names so translators know to retranslate
> them.

I agree, we should


> 2) what about using the branded string if the brand exists an the non-branded
> string if it doesn't.

This would require inventing a new "test whether brand string exists".
Not sure if such a test is straightforward. Will it always be "(null)"?

I think avoiding the brand string is a great simplification.

Comment 15

[Kai Engert [:KaiE:]]

(In reply to comment #13)
> 2) what about using the branded string if the brand exists an the non-branded
> string if it doesn't.

In addition, it would require that we keep both versions of strings.
I would like to avoid that.

Comment 16

[Kai Engert [:KaiE:]]

Well. Never mind. This bug is no longer valid.

The strings got already changed a while ago with some other bug.
The strings already read:
PSMERR_SSL_Disabled=Can't connect securely because the SSL protocol has been disabled.
PSMERR_SSL2_Disabled=Can't connect securely because the site uses an older, insecure version of the SSL protocol.

It seems we are no longer using SSL_NoMatchingCiphers, but are now using the default error strings provided by NSS.

I'm resolving this as WORKSFORME.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Sorry, I should have checked the file before I commented in comment 10 :(

Comment 18

[Axel Hecht [:Pike]]

Mind canceling the review requests, then?

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 248535 [details] [diff] [review]
Patch v1

canceling review requests that we don't need anymore...