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

Increase secure notes length to match LastPass. #3168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

STRML
Copy link

@STRML STRML commented Jan 25, 2023

As users are migrating from LastPass, I am seeing more and more people hitting the max length of 10,000 on the "Notes" field, which applies to both CipherType.Login and CipherType.SecureNote.

This default is too low, and by being significantly lower than competing products, needlessly complicates the transition.

This is not helped by how unhelpful the import process is on web (showing errors in console only, silently dropping items), which is a PR for another day. By merging this, we can ensure that one of the most common drop-offs is fixed.

45,000 matches LastPass. As we know that the encrypted length can be longer than the raw text, I have increased this to 50,000 so there is a healthy buffer to account for this inflation.

Mirrors PR at bitwarden/server#2625

@BlackDex
Copy link
Collaborator

While I'm all for increasing this limit, I'm a bit hesitant to do so because of #2937. That's why we added a limit in the first place actually.

Also, on this part i would like to keep compatibiliteit with Bitwarden in regards to export from Vaultwarden and import to Bitwarden.

So, if your PR gets accepted at Bitwarden, i see no reason not to increase it here either.

@STRML
Copy link
Author

STRML commented Jan 25, 2023

Fair, thanks for the input. I would suspect #2937 is not going to be such an issue with 50k notes versus 15M, given the 2+ order of magnitude size difference. But let's wait and see what happens at Bitwarden, as I don't want to create a compatibility issue.

@tessus
Copy link
Contributor

tessus commented Apr 19, 2023

So... bw closed the PR claiming they will come up with a different solution. So I guess this PR can either be merged or closed.

IMO allowing 50k instead of 10k is not going to hurt anyone, except that it will make the life easier for LastPass users who want to migrate to vw.

@STRML
Copy link
Author

STRML commented Apr 19, 2023

Honestly it's pretty frustrating, because it doesn't look like that solution is coming any time soon, but migrations into bw/vw happen every day.

It's such a common mistake to let the perfect be the enemy of the good.

@tessus
Copy link
Contributor

tessus commented Apr 19, 2023

Send me an email and I'll let you know what I think. I will not post this publicly.

@BlackDex
Copy link
Collaborator

BlackDex commented Jun 12, 2023

While i do not like to add more configuration options, it might be an option to add it for this specific feature. That way, people could keep the default to be compatible with Bitwarden, and increase it to be compatible with LastPass to import there vaults.

It could however be, that some clients will have issues with decrypting such values, but that could be added as a warning/know the risk message.

@dani-garcia , what do you think?

@stefan0xC
Copy link
Contributor

stefan0xC commented Sep 19, 2023

Bitwarden has apparently increased the encrypted string length to 35000 in bitwarden/server#3193 (which should be relevant to this issue)

@STRML
Copy link
Author

STRML commented Sep 19, 2023

Great, just matching the official server here will make migration much easier.

Although, arguably, the ship of mass migration has already sailed.

@BlackDex
Copy link
Collaborator

Bitwarden has apparently increased the encrypted string length to 35000 in bitwarden/server#3193 (which should be relevant to this issue)

I Would suggest we update to the same value.

@jjlin
Copy link
Contributor

jjlin commented Sep 19, 2023

Bitwarden has apparently increased the encrypted string length to 35000 in bitwarden/server#3193 (which should be relevant to this issue)

That's for their secrets manager, not for the password manager secure notes.

@stefan0xC

This comment was marked as outdated.

@stefan0xC
Copy link
Contributor

stefan0xC commented Sep 19, 2023

That's for their secrets manager, not for the password manager secure notes.

Ah, thanks. I'm apparently tired. sorry, I completely missed that that is the secrets manager

@Jayd603
Copy link

Jayd603 commented Oct 17, 2023

So... I ran into this issue. We wanted to store notes that others can edit and they happen to be larger than 10k characters. We can do an attachment for now but it creates extra steps for people, ie, download file, edit, upload again. Could this possibly be user configurable to an extent? What is the max we can do without it becoming an issue? I may experiment with it myself and will report back if I find anything interesting.

Update: I ended up doing my own patch for this for now. 100k instead of 10k. There will not be a lot of large notes so it shouldn't bother performance. Thanks for the hard work on the project everyone, it's a really nice app.

@Jayd603
Copy link

Jayd603 commented Feb 17, 2024

Bump - looks like this is the same size still. 10k seems too restrictive for a secure note on modern hardware. The Bitwarden request was rejected but it sounds like the issue is with their back-end storage/architecture due to the large number of users they have on their hosted platform. I guess i don't care as long as it remains a simple patch for us, just one more step during deployment, but other users may want larger notes without having to patch/build a docker image etc.

@mwa85
Copy link

mwa85 commented May 6, 2024

I can't migrate from LastPass do to this limit so I'll like this limit to be lifted and be the same as LastPass to make migration (importing the LastPass-export) possible

Thanks.

@tessus
Copy link
Contributor

tessus commented May 13, 2024

I don't think this PR will ever be merged, because the devs want vw to behave the same way as bw, even if bw's design is idiotic and detrimental to a successful migration from Lastpass.

However, you can always compile your own version that includes this PR.

@STRML
Copy link
Author

STRML commented May 13, 2024

It is infuriating - exactly the kind of stuff that prevents repeat contribution.

@tessus
Copy link
Contributor

tessus commented May 13, 2024

@STRML in most cases it makes sense not to divert from bw's design. But the reasoning for not adding this escapes me.

Currently people can neither migrate from Lastpass to bw or vw. (if they are using longer notes.) With this change they could migrate to vw. After that migrating from vw to bw would fail the same as it would from Lastpass to bw, so nothing new here.

bw also said they'd increase the limit some other way, so an export from vw should then be possible to be imported in bw.

The problem could be that the bw clients might not support longer notes in the future. (If their previous statement that they'd increase the limit was false.) But that's neither confirmed nor on the roadmap.

@STRML
Copy link
Author

STRML commented May 13, 2024

I don't have a business degree, but refusing to provide a hassle-free path for your competitors' users to migrate to your product feels like a miss.

If I were at BW this would have been my top priority as soon as the LP drama started.

As users are migrating from LastPass, I am seeing more and more people hitting the max length of 10,000 on the "Notes" field, which applies to both CipherType.Login and CipherType.SecureNote.

This default is too low, and by being significantly lower than competing products, needlessly complicates the transition.

This is not helped by how unhelpful the import process is on web (showing errors in console only, silently dropping items), which is a PR for another day. By merging this, we can ensure that one of
the most common drop-offs is fixed.

45,000 matches LastPass. As we know that the encrypted length can be longer than the raw text, I have increased this to 50,000 so there is a healthy buffer to account for this inflation.

Mirrors PR at bitwarden/server#2625
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.

None yet

7 participants