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

Key: permit empty keys only with ::empty() factory method #833

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

Slamdunk
Copy link
Collaborator

@Slamdunk Slamdunk commented Apr 7, 2022

I consider this a security bug that should be addressed with urgent.

Before this PR, misconfigurations can easily lead to unsecured token issuance under the radar, expecially where creator = consumer.

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 7, 2022

I'd still label it as BC break, but better to have a broken system, than a compromised one.

No need for CVE/security issue, since this is mis-configuration on the consumer side, if it happens: instructions on using safe randomly generated keys were already provided.

@Ocramius Ocramius self-assigned this Apr 7, 2022
Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @Slamdunk!

@Ocramius Ocramius changed the title Key: permit empty keys only with ::empty() factory method Key: permit empty keys only with ::empty() factory method Apr 7, 2022
@Ocramius Ocramius merged commit ae3aac8 into lcobucci:4.2.x Apr 7, 2022
@Slamdunk Slamdunk deleted the no_empty_key branch April 7, 2022 10:51
@lcobucci
Copy link
Owner

lcobucci commented Apr 8, 2022

IHMO we shouldn't use the baseline to ignore errors we will never fix. That's why we have the annotations to ignore things.

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 8, 2022

IHMO we shouldn't use the baseline to ignore errors we will never fix.

We should probably remove the exception, at some point, and only leave the types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants