search

web-push-libs / web-push-php

Web Push library for PHP
MIT License
1.72k stars 301 forks source link

Replace `spomky-labs/base64url` with `paragonie/constant_time_encoding` #397

Closed Cyperghost closed 9 months ago

Cyperghost commented 9 months ago

The composer package paragonie/constant_time_encoding is already provided by web-token/jwt-library and gives a better security.

dada-amater commented 8 months ago

Hi @Minishlink, the latest release is more than one year old. Is it possible to make a new one, please 🙏.

Minishlink commented 8 months ago

Hello, yep it is planned as soon as https://github.com/web-push-libs/web-push-php/pull/394 lands :) That PR is a breaking change so I would prefer avoiding two breaking change releases in a row (in addition of dropping PHP 8.0 support). I released a release candidate version here in the meantime https://github.com/web-push-libs/web-push-php/releases/tag/v9.0.0-rc1

sleptor commented 6 months ago

@Minishlink @Cyperghost replacing spomky-labs/base64url library was a mistake IMHO. Let me explain why.

  1. It broke backward compatibility for no reason - it stopped accepting normal (non-URL-safe) Base64 strings. In DB, I store valid base64 encoded strings, which is standard practice, IMHO) however, paragonie/constant_time_encoding can't process them and throws an exception. Moreover, you use "noPadding" functions.. why? why I have to convert valid base64 encoded string to make it working?

  2. Performance. It made the library unusable when you send thousands of messages. The fact is that paragonie/constant_time_encoding implements base64 decoding using pure PHP (hundred lines). Previous library use one-liner:
    $decoded = base64decode(strtr($data, '-', '+/'), true);

Please reconsider this change. Return old library or may be use libsodium https://www.php.net/manual/en/book.sodium.php

Minishlink commented 6 months ago

Hello, thanks for your feedback. Can you send a PR that adds a test for your use case please? (which should fail on master) Can you stick with v8 in the meantime?

sleptor commented 6 months ago

Hi, I'm using v8, and it works well. I only looked to master because of the "web-token/jwt-library" update in composer. However, I had to revert back to v8 due to backward incompatibility. I'll submit a PR as soon as possible.

Minishlink commented 6 months ago

Thank you

Minishlink commented 5 months ago

Reverted in v9.0.0-rc2