1429014 - Ensure new interfaces use [SecureContext] in test_interfaces.html

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Anne (:annevk)]

To ensure we don't inadvertently ship new features on insecure contexts unless we really meant to, Ben suggested we could modify test_interfaces.html to enforce this somehow.

(That does not address the problem for all features, but it does go some of the way there.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm not quite clear on what this bug report is asking for.

Our current testing setup is as follows:

1) dom/tests/mochitest/general/test_interfaces.html -- asserts !self.isSecureContext and then runs dom/tests/mochitest/general/test_interfaces.js

2) dom/tests/mochitest/general/test_interfaces_secureContext.html -- asserts self.isSecureContext and then runs dom/tests/mochitest/general/test_interfaces.js

Inside interfaces.js, some interfaces are annotated with "secureContext: true", and we ensure that the ones that are thus annotated are only exposed if self.isSecureContext.  We ensure that the ones that are not thus annotated are exposed in both cases.  Similarly for test_worker_interfaces.html/test_worker_interfaces_secureContext.html/test_worker_interfaces.js

Is the request that we assert an interface is exposed only in secure contexts unless it's explicitly annotated "secureContextOnly: false", as opposed to requiring a "secureContext: true" annotation?  And then we annotate pretty much everything with "secureContextOnly: false" in the tests?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Anne filed this at my suggestion.  I was suggesting that we add make changes to the file that require someone to think about why an API should be exposed to insecure contexts.  I was not aware we already had a secureContext attribute check here.

Maybe reversing the logic to defaulting requiring  secure context and then authors would have to write "allowInsecureContext:true" instead would help?  Maybe with some comments or something about this should not be permitted without good reason any more, etc.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We could certainly do that.  It's a pretty easy change.

Comment 4

[Anne (:annevk)]

Yeah, inverting the logic sounds like the way to go. Including a link to https://blog.mozilla.org/security/2018/01/15/secure-contexts-everywhere/ as well would be great.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make test_interfaces require explicit opt-in to add a

non-SecureContext interface.  r=bkelly

The big data tables had the following search-and-replaces done on them:

  name: "\([^"]+\)" -> name: "\1", insecureContext: true

  "\([^"]+\)",$ -> {name: "\1", insecureContext: true},

followed by removal of both isSecureContext and insecureContext annotations
where both appeared.

MozReview-Commit-ID: BkqAwXFf48x

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Make test_interfaces require explicit opt-in to add a non-SecureContext interface

The big data tables had the following search-and-replaces done on them:

  name: "\([^"]+\)" -> name: "\1", insecureContext: true

  "\([^"]+\)",$ -> {name: "\1", insecureContext: true},

followed by removal of both isSecureContext and insecureContext annotations
where both appeared.

MozReview-Commit-ID: BkqAwXFf48x

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8942985 [details] [diff] [review]
Make test_interfaces require explicit opt-in to add a non-SecureContext interface

Review of attachment 8942985 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/test_worker_interfaces.js
@@ +8,5 @@
>  // property giving the interface name as a string, and additional
>  // properties which qualify the exposure of that interface. For example:
>  //
>  // [
> +//   {name: "AGlobalInterface", insecureContext: true},

Should we maybe not suggest this in the comment for people modifying the file?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6dca879e6a
Make test_interfaces require explicit opt-in to add a non-SecureContext interface.  r=bkelly

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fd6dca879e6a