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"

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aryx, Assigned: ckerschb)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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"
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8643370 [details] [diff] [review]
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.
Attachment #8643370 - Flags: review?(jwalker)
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.
Flags: needinfo?(francesco.lodolo)
"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?
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.
Flags: needinfo?(francesco.lodolo)
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
Attachment #8643370 - Flags: review?(jwalker) → review+
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
Created attachment 8643787 [details] [diff] [review]
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.
Attachment #8643787 - Flags: review?(jwalker)
Attachment #8643787 - Flags: review?(jwalker) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8644419 [details] [diff] [review]
bug_1191113_csp_gcli_localization.patch

Folded patches together into one. Carrying over r+ from jwalker.
Attachment #8643370 - Attachment is obsolete: true
Attachment #8643787 - Attachment is obsolete: true
Attachment #8644419 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 10

3 years ago
I am about to land some other patch, and this one might fit in quite nicely!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2170e194fe60
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.