Closed
Bug 1430947
Opened 7 years ago
Closed 7 years ago
Navigator.credentials is not [SecureContext]
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jcj)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [webauthn] [webauthn-interop][credman])
Attachments
(1 file)
STEPS TO REPRODUCE:
1) Load a non-secure page (e.g. <http://example.com/>)
2) Open web console
3) Evaluate "navigator.credentials"
EXPECTED RESULTS: Get undefined
ACTUAL RESULTS: Get a CredentialsContainer object
ANALYSIS: https://www.w3.org/TR/credential-management-1/#framework-credential-management has a [SecureContext] bit that our Navigator.webidl is missing.
Assignee | ||
Comment 1•7 years ago
|
||
Ugh, sorry. On it, thanks!
Assignee: nobody → jjones
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: [webauthn] [webauthn-interop][credman]
Comment hidden (mozreview-request) |
![]() |
Reporter | |
Comment 3•7 years ago
|
||
Not exactly your fault, esp. given the apparent lack of web platform tests for this...
![]() |
Reporter | |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8943096 [details]
Bug 1430947 - Add [SecureContext] to navigator.credentials
https://reviewboard.mozilla.org/r/213394/#review219116
r=me; needs tests
Attachment #8943096 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
![]() |
Reporter | |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8943096 [details]
Bug 1430947 - Add [SecureContext] to navigator.credentials
https://reviewboard.mozilla.org/r/213394/#review219140
::: dom/credentialmanagement/tests/mochitest/mochitest.ini:8
(Diff revision 2)
> frame_credman_iframes.html
> scheme = https
> skip-if = !e10s
>
> [test_credman_iframes.html]
> +[test_credman_insecure_context.html]
Why not a web platform test?
Attachment #8943096 -
Flags: review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943096 [details]
Bug 1430947 - Add [SecureContext] to navigator.credentials
https://reviewboard.mozilla.org/r/213394/#review219140
> Why not a web platform test?
Sorry for the dumb question, but so far [my one commit on that repo hasn't merged yet](https://github.com/w3c/web-platform-tests/pull/7500) --
I assume I write that test externally, make a PR, and then at some point the web platform tests get merged back into m-c?
![]() |
Reporter | |
Comment 8•7 years ago
|
||
No, you can just check them directly into m-c; we do bidirectional sync.
If you're doing a large tests-only change, it can make sense to go through a PR to the wpt repo and get reviews there, but for small things that go along with code changes it's better to just land them in m-c and then they will get synced back.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Reporter | |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8943096 [details]
Bug 1430947 - Add [SecureContext] to navigator.credentials
https://reviewboard.mozilla.org/r/213394/#review219506
::: commit-message-d97ff:6
(Diff revision 4)
> +Bug 1430947 - Add [SecureContext] to navigator.credentials r?bz
> +
> +It was neglected to mark navigator.credentials as [SecureContext], yet it
> +must be for spec compliance and powerful-features compliance.
> +
> +Updated: add a web platform test
You can drop this line; the actual commit landing is not "updated" compared to what's already in the tree.
::: dom/webidl/Navigator.webidl:314
(Diff revision 4)
> [NoInterfaceObject, Exposed=(Window,Worker)]
> interface NavigatorConcurrentHardware {
> readonly attribute unsigned long long hardwareConcurrency;
> };
>
> partial interface Navigator {
Preexisting, but there should really be a link to https://w3c.github.io/webappsec-credential-management/#framework-credential-management both before this partial interface and in the list at the top of this file.
Attachment #8943096 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943096 [details]
Bug 1430947 - Add [SecureContext] to navigator.credentials
https://reviewboard.mozilla.org/r/213394/#review219506
Thanks again, Boris. Looks like the Try run was good, too. I'll upload the adjustments and get this in the landing pattern.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
Try runs are good:
1) https://treeherder.mozilla.org/#/jobs?repo=try&revision=715af764dd1cbddf05e3cd19f9b70a3a28324709
2) https://treeherder.mozilla.org/#/jobs?repo=try&revision=40097d170fd9
Marking checkin-needed.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f192d955648
Add [SecureContext] to navigator.credentials r=bz
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•