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

[PS-2351] Increase Secure Note max length to 50,000 to match LastPass. #2625

Closed
wants to merge 2 commits into from

Conversation

STRML
Copy link

@STRML STRML commented Jan 25, 2023

See https://community.bitwarden.com/t/support-longer-notes-breaks-lastpass-import/2970

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

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.

Code changes

  • src/Api/Models/Request/CipherRequestModel.cs Increase max encrypted length of Notes field to 45,000.

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2023

CLA assistant check
All committers have signed the CLA.

LastPass uses 45,000. Since buffers inflate slightly when encrypted,
a max length of 50,000 should be safe.

See https://community.bitwarden.com/t/support-longer-notes-breaks-lastpass-import/2970
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PS-2351

@bitwarden-bot bitwarden-bot changed the title Increase Secure Note max length to 50,000 to match LastPass. [PS-2351] Increase Secure Note max length to 50,000 to match LastPass. Jan 25, 2023
STRML added a commit to STRML/vaultwarden that referenced this pull request 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
@micahblut
Copy link
Member

micahblut commented Jan 25, 2023

Does this change include an update to the error message that displays when the maximum character count is exceeded?

@STRML
Copy link
Author

STRML commented Jan 25, 2023

Yes, as that is derived from the value given to the decorator.

@micahblut
Copy link
Member

Thank you for your contribution. We are currently researching new approaches for note and attachment storage that meet our security requirements and will grow well with the platform. At this time we are unable to merge this change given those long-term architectural plans for secure data storage. We hope to enable much larger notes for you soon.

@micahblut micahblut closed this Mar 10, 2023
@STRML
Copy link
Author

STRML commented Mar 10, 2023

I am sad to see the perfect be the enemy of the good here. Where is this work being done and how can we track it?

@tessus
Copy link

tessus commented Mar 10, 2023

I am also not quite sure why such an easy fix won't be merged to alleviate the pain people, who are migrating from LastPass, actually have right now.
Well, maybe Bitwarden don't won't others to migrate to their product.

BlackDex pushed a commit to STRML/vaultwarden that referenced this pull request May 19, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants