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

Update cuid implementation to use cuid2 to improve security #3827

Closed
wants to merge 4 commits into from

Conversation

Vashiru
Copy link

@Vashiru Vashiru commented Mar 29, 2023

This is a pull request for the changes I mention in #3826. Consider this a start for updating the cuid implementation to cuid2. I've made the changes as described in #3826 and updated the lockfile using Cargo build.

I am not a Rust developer, so this might very well be incomplete or incorrect and somebody else will have to take it from here. But I hope to kickstart the implementation of this by making this pull request.

@Vashiru Vashiru requested a review from a team March 29, 2023 16:05
@CLAassistant
Copy link

CLAassistant commented Mar 29, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

requesting changes to block accidental merges, we have to come to a conclusion in the prisma/prisma issue first.

@janpio janpio requested a review from a team as a code owner June 14, 2023 10:52
@SevInf SevInf added PR: Feature A PR That introduces a new feature PR: Breaking change labels Mar 20, 2024
@SevInf SevInf self-assigned this Mar 21, 2024
@SevInf
Copy link
Contributor

SevInf commented Mar 21, 2024

Hi @Vashiru.
Thank you for submitting the contribution, but I don't think we can merge it.

We can't just silently replace cuid1 with 2 - it will be a breaking change for a lot of our users.
One of the big reasons for that: cuid1 being monotonically increasing was one of it's advertised features before it was declared insecure my maintainers. So, it is very likely that there is a lot of code out there, relying one this property.

For example:

prisma.user.findMany({
  id: { gt: lastId }
})

it is expected that all records this code returns would created after lastId was. Transparently replacing cuid1 with cuid2 will break that.

Proper way forward was would be to deprecate, but keep cuid() psl function and add a separate cuid2() function.

I am going to close this PR now. Feel free to submit a new one, with non-breaking implementation of the feature.

@SevInf SevInf closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Breaking change PR: Feature A PR That introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants