Closed
Bug 1471959
Opened 6 years ago
Closed 6 years ago
TLS status API should return `null` for certain attributes
Categories
(WebExtensions :: Request Handling, defect, P1)
WebExtensions
Request Handling
Tracking
(firefox62 fixed, firefox63 fixed)
RESOLVED
FIXED
mozilla63
People
(Reporter: April, Assigned: mixedpuppy)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-review-board-request
|
rpl
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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•6 years ago
|
Flags: needinfo?(mixedpuppy)
Updated•6 years ago
|
Assignee: nobody → mixedpuppy
Priority: -- → P1
Assignee | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
Attachment #8989253 -
Flags: review?(lgreco)
Comment 5•6 years ago
|
||
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•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8989253 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 8•6 years ago
|
||
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+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14e28f7332c7
leave keaGroupName and signatureSchemeName undefined if value is none, r=rpl
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox62:
--- → affected
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 14•6 years ago
|
||
bugherder uplift |
Comment 15•6 years 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•6 years ago
|
Flags: needinfo?(mixedpuppy) → qe-verify-
Comment 16•6 years ago
|
||
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)
Updated•6 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 17•6 years 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.
Description
•