gauntface/simple-push-demo

Is the code in the master branch outdated?

FluorescentHallucinogen opened this issue ยท 11 comments

@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

// 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!

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

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

  1. Is that still required?

See also #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. ๐Ÿ˜‰

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.

Good!

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

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?

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:

pushRequest.write(body.body);

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

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.

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.

@gauntface

I've broken glitch with a memory issue.

Consider Firebase Cloud Functions. ๐Ÿ˜‰

// 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);
fopts.body = requestInfo;
}
fopts.body = JSON.stringify(requestInfo);

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?

@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.

@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.

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.

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]> (

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

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 #203.

Any plans to fix it?

Firefox worked for me using aesgcm, BUT the spec says only aes128gcm is needed. I also raised
mdn/browser-compat-data#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.