gauntface / simple-push-demo

A simple example of use push notifications on the web using Service Workers
https://simple-push-demo.vercel.app/
Apache License 2.0
883 stars 217 forks source link

Is the code in the master branch outdated? #205

Closed FluorescentHallucinogen closed 1 year ago

FluorescentHallucinogen commented 1 year ago

@gauntface While working on #204, I've found some interesting things and I have a few questions for you.

Just found that the code deployed to https://gauntface.github.io/simple-push-demo/ uses https://simple-push-demo-api.glitch.me/api/v3/sendpush, but code in the master branch uses https://simple-push-demo.appspot.com/api/v2/sendpush that responds with an error 500.

  1. Could you please update the code in this repo?

  2. Any plans to place the code of https://simple-push-demo-api.glitch.me/api/v3/sendpush to the repo?

Also found https://github.com/gauntface/simple-push-demo/blob/3b55ad9aa10800713fb37d59a425fd7e86f9def7/src/scripts/app-controller.js#L283-L289

In fact, this is not true. The body for fetch() can be ArrayBuffer. There is no need to convert it to Base64. I've tried to make the network request to the push service directly from the browser via fetch() with body of type ArrayBuffer. And it worked!

The only problem I faced is that https://fcm.googleapis.com/fcm/send/ requires CORS (this problem can be bypassed by --disable-web-security console line argument for the browser). That's why this demo requires the CORS proxy. Although the https://updates.push.services.mozilla.com/wpush/v2/ works like a charm!

  1. Any plans to drop the Base64 conversion?

Also found https://github.com/gauntface/simple-push-demo/blob/3b55ad9aa10800713fb37d59a425fd7e86f9def7/src/scripts/encryption/encryption-aes-128-gcm.js#L49-L50

  1. Is that still required?

See also https://github.com/gauntface/simple-push-demo/issues/203.

  1. Any plans to fix it?

  2. Can https://github.com/web-push-libs/web-push library be used in the browser? If so, any plans to migrate this project to using it instead of maintaining https://github.com/gauntface/simple-push-demo/tree/master/src/scripts/encryption files?

I expect more interest to this project due to the forthcoming push notifications support in Safari. :wink:

gauntface commented 1 year ago

I need to spend some time cleaning this up.

I think its using an old glitch backend and some other bits that aren't well maintained or documented. Will try and look at this next week.

FluorescentHallucinogen commented 1 year ago

Good!

Could you please then merge #204 first to avoid merge conflicts?

FluorescentHallucinogen commented 1 year ago

Oh, I've found a main branch, and it's 4 commits ahead of master branch.

The gh-pages is created from main, not master.

@gauntface Could you please remove the master branch and make the main branch default to avoid future contributors confusion?

FluorescentHallucinogen commented 1 year ago

But the server code in the main branch and the server code deployed to the https://simple-push-demo-api.glitch.me are still different. Just compare https://github.com/gauntface/simple-push-demo/blob/13eca6424e20f2b68fc35c98028d35ab3fa8e528/server/server.js and https://glitch.com/edit/#!/simple-push-demo-api?path=server.js.

The interesting part is that both codes don't work correctly. 🙈

The code deployed to the https://simple-push-demo-api.glitch.me doesn't send the body at all:

https://glitch.com/edit/#!/simple-push-demo-api?path=server.js%3A12%3A0

That's why displaying a notification for a push with payload doesn't work:

The message payload is smaller than the smallest valid message (104 bytes)

screenshot

And the code in the main branch doesn't decode the body from Base64:

https://github.com/gauntface/simple-push-demo/blob/13eca6424e20f2b68fc35c98028d35ab3fa8e528/server/server.js#L50

That's why displaying a notification for a push with payload doesn't work:

The public key included in the binary message header must be a valid P-256 ECDH uncompressed point that is 65 bytes in length.

screenshot

gauntface commented 1 year ago

Thank you for all the digging here.

This project got very little attention as the server had to moved around a lot and I didn't give it the time it needed.

I'm putting together a page that should make debugging this a little easier because even with the inclusion of the body, the request doesn't seem to be valid and comparing to web-push there are differences.

gauntface commented 1 year ago

Small update on this - I go the server working locally with payloads, however, I've broken glitch with a memory issue.

I'll probably look at a different hosting solution and GitHub -> Glitch doesn't seem particularly clean.

FluorescentHallucinogen commented 1 year ago

@gauntface

I've broken glitch with a memory issue.

Consider Firebase Cloud Functions. :wink:

https://github.com/gauntface/simple-push-demo/blob/8249042e988cbb24e98e6b906bc7f2fd4bf09dc6/frontend/scripts/app-controller.js#L259-L267

By dropping Base64 conversion I mean sending fetch() request to CORS proxy "as is" with body of type ArrayBuffer and pass the original push endpoint address in the request header, e.g. X-Endpoint. That's exactly what @beverloo did in his demo https://tests.peter.sh/push-message-generator/. In this case, there is no need to JSON.stringify and convert the body to Base64. See https://github.com/beverloo/peter.sh/blob/aa735a65e42e3d86db3d12adf72dcea666a212f6/tests/push-message-generator/push_generator.js#L779-L791.

Also, what about using cors-anywhere npm package for CORS proxy?

@beverloo Any plans to enable CORS for https://fcm.googleapis.com/fcm/send/ so it can be used directly from the browser without the need for the CORS proxy?

gauntface commented 1 year ago

@FluorescentHallucinogen I'll take a look and see if the server is needed at all. If we can stream the content to FCM then I can make the request with 'no-cors'.

I think when I first made this demo the content wasn't a stream and worked, then it required a stream but the fetch API didn't support it, so I introduced the proxy server.

I'll probably do a pass of topics and break out this bug into smaller chunks.

For now I've moved hosting to vercel.

FluorescentHallucinogen commented 1 year ago

@gauntface

I'll take a look and see if the server is needed at all. If we can stream the content to FCM then I can make the request with 'no-cors'.

Unfortunately, you can't, because you can't use headers like Authorization, Content-Encoding, etc. with no-cors. See https://stackoverflow.com/a/44748092 and https://stackoverflow.com/a/44987520 for more info.

FluorescentHallucinogen commented 1 year ago

BTW, my initial idea instead of implementing #204 for displaying and sending the payload via CURL has been displaying the fetch() code instead of CURL command.

As an idea, I could make a pull request that adds displaying the fetch() code in addition to the CURL command and a hint to add --disable-web-security console line argument for Chromium-based browsers.

gauntface commented 1 year ago

I think I've addressed most of your questions in the first batch.

Could you please update the code in this repo?

Done

Any plans to place the code of https://simple-push-demo-api.glitch.me/api/v3/sendpush to the repo?

Done

Also found

simple-push-demo/src/scripts/app-controller.js

Lines 283 to 289 in 3b55ad9

// Can't send a stream like is needed for web push protocol, // so needs to convert it to base 64 here and the server will // convert back and pass as a stream if (requestInfo.body && requestInfo.body instanceof ArrayBuffer) { requestInfo.body = this.toBase64(requestInfo.body); fetchOptions.body = requestInfo; }

In fact, this is not true. The body for fetch() can be ArrayBuffer. There is no need to >convert it to Base64. I've tried to make the network request to the push service directly >from the browser via fetch() with body of type ArrayBuffer. And it worked!

Raised separate issue for this.

The only problem I faced is that https://fcm.googleapis.com/fcm/send/ requires CORS (this >problem can be bypassed by --disable-web-security console line argument for the browser). >That's why this demo requires the CORS proxy. Although the > >https://updates.push.services.mozilla.com/wpush/v2/ works like a charm!

I'm trying to make this something where folks can learn about web push and it's encryption from the browser without needing to go anywhere else, include using specific flags or pulling our a separate tool (i.e. node). So I wouldn't encourage the use of the extra flag.

Any plans to drop the Base64 conversion?

See new bug

Also found

[simple-push-demo/src/scripts/encryption/encryption-aes-128-gcm.js]> (https://github.com/gauntface/simple-push-demo/blob/3b55ad9aa10800713fb37d59a425fd7e86f9def7/src/scripts/encryption/encryption-aes-128-gcm.js#L49-L50)

Lines 49 to 50 in 3b55ad9

if (endpoint.indexOf('https://fcm.googleapis.com') === 0) { endpoint = endpoint.replace('fcm/send', 'wp'); Is that still required?

Nope and fixed.

See also https://github.com/gauntface/simple-push-demo/issues/203.

Any plans to fix it?

Firefox worked for me using aesgcm, BUT the spec says only aes128gcm is needed. I also raised https://github.com/mdn/browser-compat-data/issues/17146 to update MDN compat table.

Can https://github.com/web-push-libs/web-push library be used in the browser? If so, any plans to migrate this project to using it instead of maintaining https://github.com/gauntface/simple-push-demo/tree/master/src/scripts/encryption files?

I don't think there are any plans to make that run in the browser and I don't think there would be much value in it because the main use case for sending push messages would be to send them from a server/backend which web-push does.

This demo is more a toy/educational tool.

That being said, if there was a way to use web-push in the browser, I would love to switch to it as it'll be more future proof (and maybe at that point the demo should be moved to the web-push-libs org).

I'm going to close this issue as I think the core problems have been fixed, but please feel free to open other issues if I've missed anything or I've introduced new problems.