Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update README for subscribing #11

Closed
wants to merge 1 commit into from

Conversation

rossta
Copy link
Contributor

@rossta rossta commented Jan 30, 2024

This change updates a long-standing piece of Push Subscription directions in the README which demonstrates setting of the applicationServerKey into a Uint8Array from the decoded raw bytes of the vapid public key.

This step is cumbersome and there is a simpler alternative.

The Push API docs for the PushSubscriptionOptions interface say of the applicationServerKey:

When provided as a DOMString, the value MUST be encoded using the
base64url encoding [RFC7515].
https://www.w3.org/TR/push-api/#dom-pushsubscriptionoptions-applicationserverkey

RFC 7515 states that the Base64 url-safe encoding omit the padding characters:

Base64 encoding using the URL- and filename-safe character set defined
in Section 5 of RFC 4648 [RFC4648], with all trailing '=' characters omitted https://www.rfc-editor.org/rfc/rfc7515

The library's generated vapid public key is already Base64 url-safe encoded. However, for historical reasons, Ruby's Base64.urlsafe_encode64 method includes the padding character, "=", incorrectly by default. This behavior exists in the library's current vapid key generation. Therefore, the directions include explicit deletion of the "=" character.

For historical discussion of Ruby's encoding issue, see: https://bugs.ruby-lang.org/issues/10740


I've tested the change in recent versions of macOS-based Chrome, Safari, Firefox, and Edge.

@collimarco
Copy link
Contributor

Thanks for this suggestion. You also need to update the sentence above the code snippet.

P.S. the current method is not so bad and it is widely tested. I don't know which one is better / more compatible.

This change updates a long-standing piece of Push Subscription
directions in the README which demonstrates setting of the
applicationServerKey into a Uint8Array from the decoded raw bytes of the
vapid public key.

This step is cumbersome and there is a simpler alternative.

The Push API docs for the PushSubscriptionOptions interface say of the
applicationServerKey:

> When provided as a DOMString, the value MUST be encoded using the
base64url encoding [RFC7515].
https://www.w3.org/TR/push-api/#dom-pushsubscriptionoptions-applicationserverkey

RFC 7515 states that the Base64 url-safe encoding omit the padding
characters:

> Base64 encoding using the URL- and filename-safe character set defined
in Section 5 of RFC 4648 [RFC4648], with all trailing '=' characters
omitted https://www.rfc-editor.org/rfc/rfc7515

It turns out the generated vapid public is already Base64 url-safe
encoded! However, for historical reasons, Ruby's Base64.urlsafe_encode64
method includes the padding character, "=", incorrectly by default. This
behavior exists in the library's current vapid key generation.
Therefore, the directions include explicit deletion of the "=" character.

For historical discussion of Ruby's encoding issue, see:
https://bugs.ruby-lang.org/issues/10740
@rossta
Copy link
Contributor Author

rossta commented Jan 30, 2024

Thanks for the comments. I rewrote with the original method left in as alternative to let the reader decide.

@collimarco
Copy link
Contributor

I see many errors in the code... Have you tried it?

There are many syntax errors: there is an extra parentheses, the string is not double quoted, some constants have the wrong case, etc.

@collimarco
Copy link
Contributor

collimarco commented Jan 30, 2024

@rossta I rewrote this pull request to be more clear and added you as a co author of the commit :)

@rossta
Copy link
Contributor Author

rossta commented Jan 30, 2024

Thanks for fixing those errors. Multi-tasking here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants