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

Problem on sending bulk pushes for real world apps #197

Open
ozgurhangisi opened this issue Nov 28, 2018 · 12 comments
Open

Problem on sending bulk pushes for real world apps #197

ozgurhangisi opened this issue Nov 28, 2018 · 12 comments

Comments

@ozgurhangisi
Copy link

NOTE: Please test in a least two browsers (i.e. Chrome and Firefox). This
helps with diagnosing problems quicker.

Please confirm the following:

  • [X ] I have read the README entirely
  • [ X] I have verified in the issues that my problem hasn't already been resolved

Setup

Please provide the following details, the more info you can provide the
better.

  • Operating System:
  • PHP Version: <7.2>
  • web-push-php Version:

Please check that you have installed and enabled these PHP extensions :

  • [X ] gmp
  • [ X] mbstring
  • [ X] curl
  • [ X] openssl

Please select any browsers that you are experiencing problems with:

  • [ X] Chrome
  • [X ] Firefox
  • [ X] Firefox for Mobile
  • [ X] Opera for Android
  • [X ] Samsung Internet Browser
  • [X ] Other

Please specify the versions (i.e. Chrome Beta, Firefox Beta etc).
All Versions

Problem

Please explain what behaviour you are seeing.

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.

Please explain what you expected to happen

Features Used

  • [ X] VAPID Support
  • [X ] GCM API Key
  • [X ] Sending with Payload

Example / Reproduce Case

Please provide a code sample that reproduces the issue. If there is a
repository that reproduces the issue please put the link here.

Other

Thanks :)

@AngryElephant
Copy link

this is interesting

@t1gor
Copy link
Contributor

t1gor commented Dec 3, 2018

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:

  1. User subscribes in browser
  2. We store subscription together with user agent string
  3. Subscriptions is being segmented in a background (e.g. user agent string is parsed; platform, browser, device info saved). This step could also validate the auth/sub object.
  4. Only after that the subscription is allowed into the pool to be used in worker that sends out messages

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.

@ozgurhangisi
Copy link
Author

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,
Ozgur.

@t1gor
Copy link
Contributor

t1gor commented Dec 3, 2018

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.

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?

@ozgurhangisi
Copy link
Author

:) 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.

@t1gor
Copy link
Contributor

t1gor commented Dec 3, 2018

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

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.

But I think sending invalid tokens to the push servers is not the best way.

Well, feel free to submit a pull-request then, I guess :)

@ozgurhangisi
Copy link
Author

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.
// if GCM
if (substr($endpoint, 0, strlen(self::GCM_URL)) === self::GCM_URL) {
if (array_key_exists('GCM', $auth)) {
$headers['Authorization'] = 'key='.$auth['GCM'];
} else {
$headers['Authorization'] = 'key='.'';
// throw new \ErrorException('No GCM API Key specified.');
}
}
line 317 we don't throw exception if parse_url fails.

            if (!$audience_arr || !isset($audience_arr['scheme']) || !isset($audience_arr['host'])) {
               // throw new \ErrorException('Audience could not be generated.');
                $audience = ''.'://'.'';
            } else {
                $audience = $audience_arr['scheme'].'://'.$audience_arr['host'];
            }

VAPID.php line 140 : return empty if VAPID Keys are wrong

    try {
        $jsonConverter = new StandardConverter();
        $jwsCompactSerializer = new CompactSerializer($jsonConverter);
        $jwsBuilder = new JWSBuilder($jsonConverter, AlgorithmManager::create([new ES256()]));
        $jws = $jwsBuilder
            ->create()
            ->withPayload($jwtPayload)
            ->addSignature($jwk, $header)
            ->build();

        $jwt = $jwsCompactSerializer->serialize($jws, 0);
        $encodedPublicKey = Base64Url::encode($publicKey);
    } catch(\Exception $e) {
        return [
            'Authorization' => 'vapid t=, k=',
            'Crypto-Key' => 'p256ecdsa='
        ];
    }

Encryption.php our file is completely different bu we added try catch block for creation of sharedSecret.

        try {
            $sharedSecret =  self::getSharedSecret($localKeyObject, Base64Url::decode($userPublicKey));
        } catch (\Exception $e) {
            return [
                'PublicKey'=>null,
                'sharedSecret'=>null
            ];
        }

@ryancco
Copy link

ryancco commented Jun 3, 2019

This would likely be an issue with the Guzzle client not being configured with the option:
['http_errors' => false]

This can be configured by passing the above array as the fourth parameter to Minishlink\WebPush\Webpush::__construct().

You can read more here.

@ozgurhangisi
Copy link
Author

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.

@plakhin
Copy link

plakhin commented Aug 7, 2020

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

@leihaocoder
Copy link

@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

@nevstas
Copy link

nevstas commented Apr 26, 2024

<?php
use Minishlink\WebPush\Encryption;

foreach ($deliveries as $delivery) {
    $subscription = Subscription::create([
        'endpoint' => $delivery[0]->endpoint,
        "keys" => [
            "auth" => $delivery[0]->key_auth,
            "p256dh" => $delivery[0]->key_p256dh
        ]
    ]);
    $payload = json_encode([
        'title' => $this->title,
        'body' => $this->body,
        'icon' => $this->icon_url,
        'data' => [
            'vibrate' => $vibrate->toArray(),
            'additionalData' => [
                'message_id' => $this->id,
                'delivery_id' => $delivery_id,
            ],
            'url' => $this->urlWithKey($delivery_id),
        ],
    ]);

    //validate keys
    try {
        $contentEncoding = $subscription->getContentEncoding();
        Encryption::encrypt($payload, $delivery[0]->key_p256dh, $delivery[0]->key_auth, $contentEncoding);
    } catch (\Exception $e) {
        continue;
    }

    $pushes[] = [
        'subscription' => $subscription,
        'payload' => $payload,
    ];
}

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 a pull request may close this issue.

7 participants