search

zaru / webpush

webpush, Encryption Utilities for Web Push protocol
MIT License
393 stars 73 forks source link

How to handle Net::HTTPBadRequest 400 InvalidTokenFormat? #52

Open NARKOZ opened 6 years ago

NARKOZ commented 6 years ago

From time to time we're getting this error:

vendor/bundle/ruby/2.4.0/gems/webpush-0.3.4/lib/webpush/request.rb:39:in `perform': host: fcm.googleapis.com, #<Net::HTTPBadRequest 400 InvalidTokenFormat readbody=true>
body:
<HTML>
<HEAD>
<TITLE>InvalidTokenFormat</TITLE>
</HEAD>
<BODY BGCOLOR="#FFFFFF" TEXT="#000000">
<H1>InvalidTokenFormat</H1>
<H2>Error 400</H2>
</BODY>
</HTML>
 (Webpush::ResponseError)
    from vendor/bundle/ruby/2.4.0/gems/webpush-0.3.4/lib/webpush.rb:42:in `payload_send'

Any reason why this isn't handled as Webpush::InvalidSubscription by the gem?

stevenharman commented 5 years ago

Seems like this could be added pretty easily: https://github.com/zaru/webpush/blob/f380b7666dcb24d757a81d6354c654d7a140163f/lib/webpush/request.rb#L29-L40

Maybe it was not a known error/response code before now?

collimarco commented 5 years ago

Does InvalidTokenFormat refers to the token in the endpoint URL? Can you try to reproduce that by changing a character inside the endpoint? If this is the case I agree that Webpush::InvalidSubscription is the right exception.

Rubioli commented 5 years ago

I did change some parts on p256dh & endpoint. On endpoint I was able to reproduce two exceptions: Net::HTTPBadRequest 400 & Webpush::Unauthorized, whereas wrong p256dh gave OpenSSL::PKey::EC::Point::Error for reason & invalid encoding.

collimarco commented 5 years ago

@Rubioli thanks for the information, however can you be more specific?

  1. Net::HTTPBadRequest 400: this is never raised directly... You will see a Webpush::ResponseError, which is the best that we can do with just the status code (400). In order to raise something specific like Webpush::InvalidSubscription we need also a specific status name... what is the name near 400? A general Bad Request or something specific?
  2. Webpush::Unauthorized is already a specific exception and this cannot be further improved
  3. "OpenSSL::PKey::EC::Point::Error for reason & invalid encoding": can you report the exact / full exceptions that you get in this case? Also with the stack trace
collimarco commented 5 years ago

I have spent some time investigating these issues, here's my results:

ID Test Push Service Response Ruby Exception
1 Add a letter inside token (Chrome) 400 UnauthorizedRegistration Webpush::Unauthorized
2 Add a letter inside token (Firefox) 404 Webpush::InvalidSubscription
3 Use an invalid URL for the endpoint URI::InvalidURIError
4 Add a letter to p256dh OpenSSL::PKey::EC::Point::Error lib/webpush/encryption.rb:17:in `initialize'
5 Add a letter to auth ArgumentError: invalid base64 lib/webpush.rb:66:in `decode64'
6 Wrong p256dh / auth, but formally valid 201
7 Use a completely wrong token (Chrome) 400 InvalidTokenFormat Webpush::ResponseError

1-2: FCM and Autopush have slightly different behaviors and this library reflects that properly. FCM doesn't even check if the endpoint exists when the VAPID key is not associated to that endpoint, and thus returns an authentication failure. Autopush on the other hand first check if the endpoint exists, and thus returns 404. 3-4-5: This library lets the specific Ruby errors propagate. For me this is ok. However if someone believes that we should wrap those errors into a more general exception (e.g. Webpush::BadSubscription), then we should create a separate issue for that. 6: This is absolutely correct. The client will fail to decrypt payload, but no error is raised because everything seems correct from server-side (you can't do anything to prevent that). 7: I agree that we can catch that specific 400 InvalidTokenFormat and return Webpush::InvalidSubscription. You can reproduce this using the endpoint https://fcm.googleapis.com/fcm/send/abc.