Closed Bug 1430947 Opened 3 years ago Closed 3 years ago

Navigator.credentials is not [SecureContext]

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bzbarsky, Assigned: jcj)

References

(Blocks 1 open bug)

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.
Ugh, sorry. On it, thanks!
Assignee: nobody → jjones
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: [webauthn] [webauthn-interop][credman]
Not exactly your fault, esp. given the apparent lack of web platform tests for this...
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 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+
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?
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 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+
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.
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f192d955648
Add [SecureContext] to navigator.credentials r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f192d955648
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.