-
Notifications
You must be signed in to change notification settings - Fork 301
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
Problem on sending bulk pushes for real world apps #197
Comments
this is interesting |
I don't really think this is a problem of the library, as the security of your application a 100% your own concern (no offence). Neither is the way you store the data. What we have as the process for our subscriptions is the following:
in your case, I guess, you're missing the CSRF-token validation on subscription endpoint, to ensure that the request is valid and should be accepted to store data. Hope that helps. |
Hi, Thanks for the comment. The real problem is if you are a push provider you generally add some code to your customer's website and run some script to get the token info. Even if you create some CSRF-token, hash, etc... a hacker can create exactly the same token like you do because your javascript is open and a hacker can easily follow the functions that you call and simulate what you do. They can also send a valid User-Agent string. I mean a hacker can easily send everything to your server endpoint. Lets say you have 499 valid data and 1 invalid token data. Even in this case you can not send any webpush notification to your subscribers. Because the library doesn't fail only for invalid token it fails for all 500 webpushes and you can not send any webpushes to your subscribers. On the third topic you mentioned above, I couldn't see any validation function in the library. If so I would be happy if you share with me. Thanks, |
For sure, because I didn't write it :) What you could also do, for example, is sending a test or welcome message straight on subscription. If that fails, just don't save the subscription info. Would that work for you? |
:) If our customer doesn't want to show anything on registration we can not send welcome message to the users. We are not the owner of the subscribers. We are a service provider. Actually we solve the problem in a different way. We add a few try catch block to the prepare and encrypt method and if error occurs we send empty headers to push servers and push servers gives Invalid Token as a result. So we act them like expired token. It's a quick and dirty fix :) But I think sending invalid tokens to the push servers is not the best way. |
Could you please post a code sample here, just so that anyone else who encounters the same issue could use it? I mean while there is no better solution.
Well, feel free to submit a pull-request then, I guess :) |
So actually we changed the code so heavily. But I will try to add the code blocks we change :) I hope it helps. WebPush.php line 303 we don't throw an exception if GCM API Key is empty.
VAPID.php line 140 : return empty if VAPID Keys are wrong
Encryption.php our file is completely different bu we added try catch block for creation of sharedSecret.
|
This would likely be an issue with the Guzzle client not being configured with the option: This can be configured by passing the above array as the fourth parameter to You can read more here. |
Hi @ryancco I don't think it's related with http_errors parameter because the problem occurs before http request. If endpoint or push keys are not correct library fails on encryption or prepare method just before sending WebPushes to push provider. The problem must be solved before http request also. BTW We sent billions of web pushes and we haven't seen any problem on http request. Of course if there is a parameter to suppress http errors you can set it by default but the main problem is in encrypt and prepare methods. |
@ozgurhangisi to send large amounts of pushes (more than 1 million) what value do you use for flush? do you use multi curl? how much RAM does your sever have? I'm trying to solve issues with sending bulk pushes to large amount of subs and at the moment able to send to ~500 subs in one call which takes about 12-15 seconds to complete, so sending to 1 million of subs takes at least 8 hours and that's pretty much. With FCM I was able to send to 1M in less than 1 minute (without delivery reports, but that is not what I actually care about at the moment) using 10 worker processes with multi curl, but with web push I'm constantly running in different issues: trying to allocate too much memory, too many files open, to long execution... I'm trying to find optimal values. |
what do you do for "With FCM I was able to send to 1M in less than 1 minute" , I have same issue with "sending to 1 million of subs takes at least 8 hours", thanks |
|
I am doing the same thing as a push provider. My solution is to double confirm the subscription. |
NOTE: Please test in a least two browsers (i.e. Chrome and Firefox). This
helps with diagnosing problems quicker.
Setup
Problem
When we send bulk webpushes (lets say 500 webpushes in 1 flush call) if any of the publickey or token is wrong it fails for all webpushes. When you get endpoint info from the browser some people can see the URL that you use to save the data on server side and they can add whatever they want.
Lets say a hacker added millions of fake endpoint or wrong public key or tokens to your system. So you also have many valid subscription data. It's impossible to send WebPushes even to valid subscriber in this case. You must find wrong public keys or auth keys manually and you must delete them manually from your db. It's impossible if you have millions of subscribers data.
Expected
I think WebPush library shouldn't throw any exception on prepare or encrypt method. It can silently add some info to the result and when we get result we can see the error and eliminate these fake data by adding some code like we do for expired endpoints.
Another option is you can add some methods like validate_endpoint_data and we can control the data using this method before adding it to db.
Features Used
Example / Reproduce Case
Other
Thanks :)
The text was updated successfully, but these errors were encountered: