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

Chore/credential migration #2562

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RubenSmn
Copy link
Contributor

@RubenSmn RubenSmn commented Feb 8, 2025

New version of storing the credentials. Allows for storing the whole URL including, protocols and all subdomains.

The Migrator will run on startup check the version of the built-in store and updates the credentials to the new format if necessary.

For the migration it will

  • check if a url matches any in the history, if yes, use that url, else
  • check if the url already has a protocol if not, add https:// protocol
  • finally store the new credentials and update the version

The same is done for the credentials that should never be saved

I've used the origin property of the URL since this one gives us the whole 'base' url so we don't need extra fields to save the protocol and subdomains.

@PalmerAL
Copy link
Collaborator

PalmerAL commented Feb 9, 2025

Thanks, I think this approach looks good generally. I haven't fully tested this yet, but a couple of initial comments:

  • In the keychain import, the saved field is this: domain: new URL(domainWithProtocol).origin,, but in the migrator, the equivalent field is url: historyEntry.url. Is your intent to save an origin or a complete page URL? (I think origin should be fine)
  • Doing this isn't secure: matchHost.replace('www.', '') === domain || matchHost === domain - imagine comparing "example.com" and "example.www.com". The www should be removed only if it appears at the start, immediately after a protocol.
  • We'll need a change for Bitwarden as well (like I did here).

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.

2 participants