Closed Bug 1470516 Opened 6 years ago Closed 6 years ago

webRequest.getSecurityInfo should not return localised values

Categories

(WebExtensions :: Request Handling, defect, P1)

defect

Tracking

(firefox62+ fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 + fixed
firefox63 --- fixed

People

(Reporter: Will, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180605171542 Steps to reproduce: See https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107 and the following comment tagged as spam showing localised values for validity and keyUsages. Given that the primary consumer of the webRequest.getSecurityInfo API is addon code, it should be easy to parse the data. Actual results: A relevant snippet of the example security info object referred to above: "validity": { "startGMT": "Donnerstag, 8. Februar 2018, 00:00:00", "endGMT": "Donnerstag, 2. Mai 2019, 12:00:00" }, "keyUsages": "unterzeichne,Schlüssel-Verschlüsselung", Mock addon code failing to make use of the security info object: Date.parse(cert.validity.startGMT) // NaN Date.parse(cert.validity.endGMT) // NaN cert.keyUsages.toLowerCase().split(',').includes('signing') // false Expected results: Non-localised example snippet: "validity": { "startGMT": "08 February 2018, 00:00:00 GMT", "endGMT": "02 May 2019, 12:00:00 GMT" }, "keyUsages": "Signing,Key Encipherment", Mock addon code successfully making use of the security info object: Date.parse(cert.validity.startGMT) // 1518048000000 Date.parse(cert.validity.endGMT) // 1556798400000 cert.keyUsages.toLowerCase().split(',').includes('signing') // true
Flags: needinfo?(mixedpuppy)
The interfaces we're using are indeed documented that they are localized strings. For validity we could probably add a timestamp since nsIX509CertValidity has a PRTime for these. For keyUsages, I don't see any alternate non-localized access to that. You would probably have to get the rawDER and use a js library to parse it.
Flags: needinfo?(mixedpuppy)
Yeah, having it return a Javascript date object would be great. As for keyUsages, could we use an enum based on what the X.509 specification lists: KeyUsage ::= BIT STRING { digitalSignature (0), nonRepudiation (1), -- recent editions of X.509 have -- renamed this bit to contentCommitment keyEncipherment (2), dataEncipherment (3), keyAgreement (4), keyCertSign (5), cRLSign (6), encipherOnly (7), decipherOnly (8) } So keyUsages === "Signing, Key Encipherment" would return [0, 2]?
(In reply to April King [:April] from comment #2) > As for keyUsages, could we use an enum based on what the X.509 specification > lists: Is there a xpcom interface to the non-string value for keyUsages? I didn't see one. It should be split out to a separate bug to get that part done.
Not that I know of. In the meantime, for people that need to know what the localized strings map to, I believe this is where they arise from: https://dxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#132
Assignee: nobody → mixedpuppy
Keywords: dev-doc-needed
Priority: -- → P1
Blocks: 1322748
Comment on attachment 8989238 [details] Bug 1470516 - remove or fix localized values in securityInfo, https://reviewboard.mozilla.org/r/254288/#review261126
Attachment #8989238 - Flags: review?(lgreco) → review+
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ce1755e40f5b remove or fix localized values in securityInfo, r=rpl
Comment on attachment 8989238 [details] Bug 1470516 - remove or fix localized values in securityInfo, Approval Request Comment [Feature/Bug causing the regression]: webRequest securityInfo [User impact if declined]: extensions may be unable to perform some security checks on requests, user impact is dependent on what extension features are supported by this. securityInfo is a new api, we want to fix this issue prior to release. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: minimal change tested on unit tests [String changes made/needed]: none
Attachment #8989238 - Flags: approval-mozilla-beta?
Checking in with the l10n team since uplifting this means string changes on beta. flod, what do you think?
Flags: needinfo?(francesco.lodolo)
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9) > Checking in with the l10n team since uplifting this means string changes on > beta. > flod, what do you think? There are no string changes in this bug. The discussion is about showing localized dates, but no real l10n changes, so it can go ahead.
Flags: needinfo?(francesco.lodolo)
Considering that https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107 is part of a bug that has the "qe-verify-" flag set and https://bugzilla.mozilla.org/show_bug.cgi?id=1470516#c8 can we set the "qe-verify-" flag for this issue as well? Just making sure since this seems to be a security issue.
Flags: needinfo?(mixedpuppy)
(In reply to marius.santa from comment #12) > Considering that https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107 > is part of a bug that has the "qe-verify-" flag set and > https://bugzilla.mozilla.org/show_bug.cgi?id=1470516#c8 can we set the > "qe-verify-" flag for this issue as well? > Just making sure since this seems to be a security issue. It's not a security issue, and it has a test added with the change.
Flags: needinfo?(mixedpuppy) → qe-verify-
Comment on attachment 8989238 [details] Bug 1470516 - remove or fix localized values in securityInfo, Fixes in support of new API, let's uplift for beta 7 so that these extensions will work with 62.
Attachment #8989238 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1474626
I can't find anywhere in the documentation that it says these are localized strings. If we aren't saying the value is localized, is this just a release notes comment?
Flags: needinfo?(mixedpuppy)
It looks like docs were done after the fix, and uplifted, so I don't think anything needs to happen here.
Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: