[audioworklet] Enforcing SecureContext on audioWorklet.addModule()

[hoch]

I don't think we have a broader consensus on this, but it might be worth to discuss it in our WG. Here are related issues from TAG and CSS Houdini TF:
w3ctag/design-principles#75
w3c/css-houdini-drafts#379

In short, the argument is that HTTPS should be enforced on audioWorklet.addModule().

I generally agree with this idea because running JS code on the higher priority thread (audio rendering thread) should take extra caution. We might want to see this as a feature rather than a restriction.

In terms of the spec work, this work needs to be done in the Worklet - not in its variants (Paint/Audio/Animation...). However, if we find a potential issue on it we want to raise a flag before it's too late.

[joeberkovitz]

We're closing since we've discussed and it falls to Worklet to decide how this goes... our spec doesn't have to change,

[hoch]

Houdini WG issue: w3c/css-houdini-drafts#505

[annevk]

I think this should be reopened since it does directly affect your specification. E.g., we shold not expose self.audioWorklet on insecure contexts.

[joeberkovitz]

@annevk Is there a clear writeup of what paint worklets are doing re: this issue? It'd be easier than reverse engineering the intent from Chromium diffs. My guess is that we add [SecureContext] to the audioWorklet attribute definition in AudioContext but I didn't see a flat-out statement in the discussion minutes or bug threads.

Reopening to make sure we track this.

[annevk]

From https://groups.google.com/a/chromium.org/d/msg/blink-dev/Jex3idOld48/VeOxMUNDBAAJ:

Ship CSS Paint in secure contexts only, and also block future types of worklets (e.g. audio worklet) on secure contexts.

I can state that this also goes for Mozilla.

[annevk]

(You didn't hit reopen btw.)

[hoch]

E.g., we shold not expose self.audioWorklet on insecure contexts.

@annevk Now we have AudioWorklet interface (which extends Worklet) and it should have the attribute of [SecureContext]. I believe that's good enough.

@joeberkovitz We can add this change to PR #1445 since it's really tiny.

[annevk]

I believe that's good enough.

The accessor for the worklet itself should have the extended attribute too. Otherwise, what's the processing model for the getter of that accessor in insecure contexts?

[hoch]

I see. Just to confirm: is this correct?

[SecureContext]
interface AudioWorklet : Worklet {}

partial interface BaseAudioContext {
  [SecureContext] readonly attribute AudioWorklet audioWorklet;
}

[inexorabletash]

Yes, that looks correct to me.

EDIT: Take that with a grain of salt... the spec doesn't currently have an audioWorklet attribute in BaseAudioContext, so I was reacting to tossing the annotation on such an interface member as if it existed.

[inexorabletash]

... although perhaps AudioWorkletNode should be restricted to [SecureContext] as well, as it's not useful otherwise?

[hoch]

AudioWorkletNode won't be instantiable because there is no way to load the script code via AudioWorklet. Would it still be necessary?

Hmm. Perhaps. The error message will suggests something like "the definition is not registered in AudioWorklet". Referring an object that's not instantiable sounds a bit off.

[inexorabletash]

Sorry, revising my comment above, it should be:

partial interface Window {
    [SameObject, SecureContext]
    readonly attribute Worklet audioWorklet;
};

No need to add anything to BaseAudioContext, assuming the current spec is correct about where the audioWorklet attribute lives.

[hoch]

Oh, we're moving AudioWorklet under BaseAudioContext.

Thread: #1435

[inexorabletash]

Thanks for clarifying!. :) Then yeah, as you wrote above.

But if you're really specing it as a partial then this would be preferred:

[SecureContext]
partial interface BaseAudioContext {
  readonly attribute AudioWorklet audioWorklet;
}

... to imply that the partial is not applied in non-secure contexts. If you just used partial for a short explanation in the bug, then applying it directly to the attribute is preferred.

[inexorabletash]

Referring an object that's not instantiable sounds a bit off.

Agreed. IMHO, the API surface should make sense in each type of context. Having a type exposed in a non-secure context which can't possibly be used there seems weird.

[hoch]

If you just used partial for a short explanation in the bug, then applying it directly to the attribute is preferred.

Yes. That was my intention.

Agreed. IMHO, the API surface should make sense in each type of context. Having a type exposed in a non-secure context which can't possibly be used there seems weird.

Okay, I will raise the issue in the meeting soon. Adding [SecureContext] on AudioWorklet, BAC.audioWorklet and AWN make sense to me.

[joeberkovitz]

I have made these changes in #1445. Reopening this bug and connecting to that PR as the fix.