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

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

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

Cyperghost
Copy link
Contributor

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

composer.json Outdated
@@ -34,8 +34,7 @@
"ext-mbstring": "*",
"ext-openssl": "*",
"guzzlehttp/guzzle": "^7.4.5",
"web-token/jwt-library": "^3.3.0",
"spomky-labs/base64url": "^2.0.4"
"web-token/jwt-library": "^3.3.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it best practive nowadays in PHP to use sub dependencies in the project? Shouldn't the dependency be added as a direct dependency so that if jw-library removes paragonie/constant_time_encoding on a minor/patch version, the project still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I forgot to add that.

Copy link
Member

@Minishlink Minishlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@Minishlink Minishlink merged commit 2c24f4c into web-push-libs:master Mar 4, 2024
9 checks passed
@dada-amater
Copy link

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

@Minishlink
Copy link
Member

Hello, yep it is planned as soon as #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
Copy link
Contributor

sleptor commented May 11, 2024

@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 = base64_decode(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
Copy link
Member

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
Copy link
Contributor

sleptor commented May 13, 2024

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
Copy link
Member

Thank you

@Minishlink
Copy link
Member

Reverted in v9.0.0-rc2

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.

4 participants