1191113 - gcli's 'security csp' has hardcoded English (non-localizable, should be) error message if no CSP found: "Could not find any 'Content-Security-Policy' for"

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox Nightly 42.0a1 20140804 and Aurora 41.0a2

gcli's 'security csp' has a hardcoded English (non-localizable, should be) error message if no CSP is found: "Could not find any 'Content-Security-Policy' for"

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191113_csp_gcli_localization.patch

Ah, sorry I missed that. We should uplift that patch as well. I will prepare a patch for dev edition and beta.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This patch hasn't made it yet to beta. As this would break string freeze on Aurora and is something normal users won't see, I recommend to wontfix it on that branch. NI flod for confirmation.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

"Content-Security-Policy for: <host>" is also not localized:
   2.139 +          "    <td> ${csp.header} for: <b>" + uri + "</b><br/><br/>" +

Do you want a new bug for that?

Comment 4

[Francesco Lodolo [:flod]]

While the final decision for uplift is up to release-drivers, I strongly advise against breaking string freeze in aurora and beta. 

Some exceptions might make sense at the beginning of the cycle, but it's better to live with an hard-coded string for one cycle than dealing with the noise we create by landing the string.

Also note that we're 5 days from merge day, so it's probably late for any patch to land on beta unless things are utterly broken.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 8643370 [details] [diff] [review]
bug_1191113_csp_gcli_localization.patch

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

::: toolkit/devtools/gcli/commands/security.js
@@ +144,2 @@
>              "  </tr>" +
>              "</table>"});

This is an FYI, not a request for change:
You could also do:

    return context.createView({
      html:
        "<table class='gcli-csp-detail' ... valign='top'>" +
        "  <tr>" +
        "    <td><img src='.../gcli_sec_bad.svg' .../> </td> " +
        " <td>${l10n.securityCSPNoCSPOnPage} ...</td>" +
        "  </tr>" +
        "</table>",
      data: { l10n: l10n.propertyLookup }
    });

Using l10n.propertyLookup means you don't need to get the NO_CSP_ON_PAGE_MSG const

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Aryx [:archaeopteryx][:aryx] from comment #3)
> "Content-Security-Policy for: <host>" is also not localized:
>    2.139 +          "    <td> ${csp.header} for: <b>" + uri +
> "</b><br/><br/>" +
> 
> Do you want a new bug for that?

Let me prepare a new patch including that change as well, no need for a new bug.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #5)
> Comment on attachment 8643370 [details] [diff] [review]
> bug_1191113_csp_gcli_localization.patch
> 
> Review of attachment 8643370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/gcli/commands/security.js
> @@ +144,2 @@
> >              "  </tr>" +
> >              "</table>"});
> 
> This is an FYI, not a request for change:
> You could also do:
> 
>     return context.createView({
>       html:
>         "<table class='gcli-csp-detail' ... valign='top'>" +
>         "  <tr>" +
>         "    <td><img src='.../gcli_sec_bad.svg' .../> </td> " +
>         " <td>${l10n.securityCSPNoCSPOnPage} ...</td>" +
>         "  </tr>" +
>         "</table>",
>       data: { l10n: l10n.propertyLookup }
>     });
> 
> Using l10n.propertyLookup means you don't need to get the NO_CSP_ON_PAGE_MSG
> const

Thanks - I will incorporate that change in the next iteration of the devtool when we are going to extend the functionality of it.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191113_csp_gcli_localization2.patch

Joe, once r+ed I will fold that patch into the other one. Those are just the new changes on top.

I figured it makes sense to have a unified behavior whenever we encounter a CSP or not. For that reason, I also removed the ":" after "for:".

Sorry I missed those hardcoded strings the first time around when we landed the devtool.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191113_csp_gcli_localization.patch

Folded patches together into one. Carrying over r+ from jwalker.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

I am about to land some other patch, and this one might fit in quite nicely!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2170e194fe60

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2170e194fe60