Add content-encoding header - Mozilla Push Server requires it (Firefo…

googlearchive / web-push-encryption

[Deprecated] Encryption Utilities for Web Push protocol

Apache License 2.0

84 stars 23 forks source link

Add content-encoding header - Mozilla Push Server requires it (Firefo… #28

Closed nguyendanv closed 8 years ago

nguyendanv commented 8 years ago

…x 47)

Was messing around with getting my push notifications working in Firefox. Firefox 47 creates a subscription object similar to Chrome 50's:

{
  endpoint: ...,
  keys: {
    p256dh: ...,
    auth: ...
  }
}

Code seems to work as is for Firefox 47 and Mozilla Push Service (and still looks fine for Chrome) with this one field added to the header.

googlebot commented 8 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

nguyendanv commented 8 years ago

Signed the CLA.

googlebot commented 8 years ago

CLAs look good, thanks!

gauntface commented 8 years ago

I have some concerns because I don't know what the effect of this is in terms of how Chrome and the different versions of firefox treat this + I had a recollection that this header changed in one version of the FF or Chrome that needed it to be aesgcm256 or something similar.

Would ideally want some automated tests to step through each browser to test what is and isn't possible.

@wibblymat you know more about this than I do.

wibblymat commented 8 years ago

LGTM - I thought this was already in, to be honest.

You can see the equivalent in the Go version

wibblymat commented 8 years ago

BTW, in response to Matt's concerns, the value 'aesgcm' indeed won't work for older versions of FF, and supporting those browsers will need a different code path.

The reason it wasn't in the code to start with was because at one point GCM would actually reject any message that included this header, regardless of value, but that's been fixed.