TLS status API should return `null` for certain attributes

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: April, Assigned: mixedpuppy)

Tracking

({dev-doc-complete})

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

Last year
In the current TLS status API, `keaGroupName` and `signatureSchemeName` return the string `"none"` if they are unset. `"none"` is kind of a magic word that doesn't really have any technical meaning, and makes it hard to test for truthiness.

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/SecurityInfo.jsm#140

My proposal:

If either `keaGroupName` or `signatureSchemeName` are set to `"none"` by `SSLStatus`, set their value to either `null` or the empty string in what is returned by the API.

The first option is more semantically correct, the latter keeps the type the same, but both are easier to test for truthiness.
Reporter

Updated

Last year
Flags: needinfo?(mixedpuppy)
Assignee: nobody → mixedpuppy
Priority: -- → P1
Assignee

Comment 1

Last year
I'm not sure I'm convinced this is an issue.  I'm also unsure about testing this value.  If I'm checking for "none", and someone changes the platform to something else...in any case will put up a patch.
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)

Comment 3

Last year
Since these are possible uplift candidates for 62, can you please provide some STR and the proper webextension(if required)? So that we can get ahead with testing as per when the bug will be fixed.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 4

Last year
I'm still debating whether/how to automate a test on this.  It is certainly not something easily testable from QA.
Flags: needinfo?(mixedpuppy)
Assignee

Updated

Last year
Attachment #8989253 - Flags: review?(lgreco)

Comment 5

Last year
mozreview-review
Comment on attachment 8989253 [details]
Bug 1471959 - leave values undefined if value is none,  rpl

https://reviewboard.mozilla.org/r/254306/#review261912

To be honest I'm not super thrilled by this, it still seems to me that "normalizing 'none' to null/undefined" could be easily done from the extension code, especially given that the implementation is mostly just returning the strings that the internal implementation is giving us and that we don't have any automated test for the property that we are going to "normalize".

Having said that, I completely trust Shane judgment and I'll leave to him the final choice if this is a change that we absolutely want to land.
Attachment #8989253 - Flags: review?(lgreco) → review+

Comment 6

Last year
mozreview-review
Comment on attachment 8989253 [details]
Bug 1471959 - leave values undefined if value is none,  rpl

https://reviewboard.mozilla.org/r/254306/#review261916

::: commit-message-6a922:1
(Diff revision 1)
> +Bug 1471959 - leave values undefined if value is none, r? rpl

Nit, let's mention somewhere in the commit message that with "values" we are referring to the `keaGroupName` and `signatureSchemeName` properties that are part of the webRequest API.
Assignee

Updated

Last year
Attachment #8989253 - Attachment is obsolete: true
Assignee

Updated

Last year
Keywords: dev-doc-needed

Comment 8

Last year
mozreview-review
Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

https://reviewboard.mozilla.org/r/255324/#review262174
Attachment #8990291 - Flags: review?(lgreco) → review+

Comment 9

Last year
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14e28f7332c7
leave keaGroupName and signatureSchemeName undefined if value is none, r=rpl
Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

Approval Request Comment
[Feature/Bug causing the regression]: 1322748
[User impact if declined]: api cleanup before api hits release, would only affect extension devs
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[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?]: minor fix
[String changes made/needed]: none
Attachment #8990291 - Flags: approval-mozilla-beta?
Some way of testing or verifying would be nice before uplift, even for a minor fix. It seems odd to say, we can't test it, and don't have any automated tests, so let's ship it faster.
Blocks: 1322748

Comment 12

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/14e28f7332c7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

Well, let's see how this does on beta 8, since this is a new API, maybe extension authors who are commenting in the meta bug 1322748 will be testing with beta.
Attachment #8990291 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

11 months ago
If manual testing is not needed please set the “qe-verify -“ flag, otherwise please provide some STR and extension if needed, in order to validate the bug.

Thanks!
Flags: needinfo?(mixedpuppy)
Assignee

Updated

10 months ago
Flags: needinfo?(mixedpuppy) → qe-verify-
These properties weren't documented before, I guess because they weren't in the schema. I've added them to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/SecurityInfo, and marked them optional, which implicitly indicates that they are undefined if not specified, which I think is the truth.

I just used the description from the schema: I didn't think it was very helpful but wasn't able to come up with anything better, even after a bit of digging. It seems like they are internal names, rather than names from any standard. But I'd be very happy to accept suggestions for improving this.

Anyway, please let me know if this covers it or we need anything else.
Flags: needinfo?(april)
Reporter

Comment 17

9 months ago
This works for me, thank you so much everyone.  :)
Flags: needinfo?(april)
You need to log in before you can comment on or make changes to this bug.