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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: collimarco91, Assigned: lina)

Tracking

({dev-doc-complete})

51 Branch
mozilla54
x86
macOS
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
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.
Reporter

Comment 1

2 years ago
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]
Reporter

Updated

2 years ago
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
Reporter

Comment 2

2 years ago
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
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.
Flags: needinfo?(collimarco91)
Reporter

Comment 4

2 years ago
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.
(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.
Assignee: nobody → kit
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(collimarco91)

Comment 7

2 years ago
mozreview-review
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(..);
}
Attachment #8834710 - Flags: review?(amarchesini) → review+
Assignee

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

2 years ago
Pushed by kcambridge@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d0174f989cfd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee

Updated

2 years ago
Keywords: dev-doc-needed
Summary: [Web Push API, VAPID] InvalidAccessError: "Invalid raw ECDSA P-256 public key.", but the key works on Chrome → Ensure array buffers and Base64-encoded strings can be passed as app server keys
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!
Thanks, Chris!
You need to log in before you can comment on or make changes to this bug.