1337348 - Ensure array buffers and Base64-encoded strings can be passed as app server keys

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[collimarco91]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.3.12 (KHTML, like Gecko) Version/10.0.2 Safari/602.3.12

Steps to reproduce:

I have generated the applicationServerKey (for pushManager.subscribe) as described here:

http://stackoverflow.com/questions/42079185/generate-the-vapid-public-key-in-rails-and-pass-it-to-javascript



Actual results:

pushManager.subscribe throws this exception:

DOMException [InvalidAccessError: "Invalid raw ECDSA P-256 public key."
code: 15
nsresult: 0x8053000f]


Expected results:

It should have accepted the public key, because it is correct.

Chrome accepts it and I could successfully send a notification with VAPID to FCM.

Comment 1

[collimarco91]

This is the Uint8Array that represents the public key:

[4, 40, 169, 137, 190, 138, 168, 242, 241, 191, 237, 4, 210, 40, 233, 112, 233, 183, 243, 140, 60, 247, 32, 220, 149, 48, 26, 114, 119, 102, 9, 13, 41, 246, 108, 108, 200, 69, 110, 218, 172, 5, 214, 255, 67, 154, 102, 208, 195, 76, 188, 74, 15, 163, 173, 232, 35, 51, 34, 64, 32, 158, 222, 20, 86]

Comment 2

[collimarco91]

Another note: in the docs you don't provide any example about the applicationServerKey and it is still reported as not supported for Firefox in the compatibility table at the bottom of the page (however I think it is actually supported, since I get that exception).

https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe

Comment 3

[Lina Butler [:lina]]

Hmm, how are you passing the key to `pushManager.subscribe`? I just tried this, and got a subscription with a matching key:

const applicationServerKey = new Uint8Array([
  4, 40, 169, 137, 190, 138, 168, 242, 241, 191, 237, 4, 210, 40,
    233, 112, 233, 183, 243, 140, 60, 247, 32, 220, 149, 48, 26, 114,
    119, 102, 9, 13, 41, 246, 108, 108, 200, 69, 110, 218, 172, 5, 214,
    255, 67, 154, 102, 208, 195, 76, 188, 74, 15, 163, 173, 232, 35,
    51, 34, 64, 32, 158, 222, 20, 86
]);
registration.pushManager.subscribe({
  applicationServerKey: applicationServerKey,
}).then(subscription => {
  let actualKey = subscription.options.applicationServerKey;
  if (!actualKey) {
    throw new Error("Subscription created without key");
  }
  for (let i = 0; i < actualKey.length; i++) {
    if (actualKey[i] != applicationServerKey[i]) {
      throw new Error("Mismatched keys");
    }
  }
  return subscription;
}).then(subscription => {
  console.log("Created subscription with matching key", subscription);
});

`applicationServerKey` landed in Firefox 48, but it looks like bug 1247685 still has "dev-doc-needed". I'll see if I can find some time to update the docs.

Comment 4

[collimarco91]

Thank you very much!!

I was using:

var bytes = new Uint8Array(len);
registration.pushManager.subscribe({
  applicationServerKey: bytes.buffer
});

Now I have removed `.buffer` and it works both on Chrome and Firefox:

var bytes = new Uint8Array(len);
registration.pushManager.subscribe({
  applicationServerKey: bytes
});

I don't know if you should also accept `bytes.buffer` or if it worked on Chrome by chance.

Comment 5

[Lina Butler [:lina]]

(In reply to collimarco91 from comment #4)
> I don't know if you should also accept `bytes.buffer` or if it worked on
> Chrome by chance.

Good catch, you uncovered a bug in Firefox! We should accept array buffers and views, but the array buffer handling was broken for the main-thread `PushManager` implementation.

While I'm here, I'll change `applicationServerKey` so that it accepts a Base64-encoded key, too. This was added in https://github.com/w3c/push-api/pull/227.

Comment 6

[Lina Butler [:lina]]

Attachment: Bug 1337348 - Ensure array buffers and Base64-encoded strings can be passed as app server keys.

Review commit: https://reviewboard.mozilla.org/r/110548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110548/

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8834710 [details]
Bug 1337348 - Ensure array buffers and Base64-encoded strings can be passed as app server keys.

https://reviewboard.mozilla.org/r/110548/#review111906

::: dom/push/Push.js:134
(Diff revision 1)
> +        );
> +      }
> +    } else if (this._window.ArrayBuffer.isView(appServerKey)) {
> +      key = appServerKey.buffer;
> +    } else {
> +      key = appServerKey;

write a comment about what this could be. Probably nothing, right? Then throw.

::: dom/push/PushManager.cpp:583
(Diff revision 1)
>      p->MaybeReject(NS_ERROR_DOM_PUSH_ABORT_ERR);
>      return p.forget();
>    }
>  
>    nsTArray<uint8_t> appServerKey;
>    if (!aOptions.mApplicationServerKey.IsNull()) {

WasPassed

::: dom/push/PushManager.cpp:625
(Diff revision 1)
> +                                                aAppServerKey);
> +    } else {
> +      MOZ_CRASH("Uninitialized union: expected string, buffer, or view");
> +    }
> +    if (!ok) {
> +      return NS_ERROR_DOM_PUSH_INVALID_KEY_ERR;

what about if:

if (aSource.IsString()) {
  ...
} else if (aSource.IsArrayBuffer()) {
  if (!PushUtil::CopyArrayBufferToArray(aSource.GetAsArrayBuffer(),
                                        aAppServerKey)) {
    return NS_ERROR_DOM_PUSH_INVALID_KEY_ERR;
  }
} else if (aSource.IsArrayBufferView()) {
  if (!PushUtil::CopyArrayBufferViewToArray(aSource.GetAsArrayBufferView(),
                                            aAppServerKey)) {
    return NS_ERROR_DOM_PUSH_INVALID_KEY_ERR;
  }
} else {
  MOZ_CRASH(..);
}

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8834710 [details]
Bug 1337348 - Ensure array buffers and Base64-encoded strings can be passed as app server keys.

https://reviewboard.mozilla.org/r/110548/#review111906

> write a comment about what this could be. Probably nothing, right? Then throw.

Good catch, added a comment that it's an array buffer.

> WasPassed

`mApplicationServerKey` is a `mozilla::dom::Nullable`; looks like it doesn't have a `WasPassed` method.

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8834710 [details]
Bug 1337348 - Ensure array buffers and Base64-encoded strings can be passed as app server keys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110548/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0174f989cfd
Ensure array buffers and Base64-encoded strings can be passed as app server keys. r=baku

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Chris Mills [:cmills]]

I've documented this by adding a sentence to the Subscribe page to clarify what data types are accepted for the key:

https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe#Parameters

I've also added a note to the Fx55 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#Service_WorkersPush 

Let me know if this looks OK, or if you need anything more. Thanks!

Comment 13

[Lina Butler [:lina]]

Thanks, Chris!