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

Passkey improvements #2121

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Passkey improvements #2121

merged 2 commits into from
Mar 4, 2024

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Feb 19, 2024

Makes some improvements to the Passkeys implementation:

  • Respects the sameOriginWithAncestors. If the origin does not match, the request are ignored.
  • Only basic checks are done here. Everything else is moved to KeePassXC side to ensure it's safer to use from 3rd party clients.
  • Timeouts are already set here instead of KeePassXC.
  • Adds a lifetime timer (with a greater timeout than the actual request). When credential creation is requested and some unexpected error happens or credential is excluded, the timer will run out. This prevents possible requests from a fraudulent site that tries to figure out which credentials are registered.
  • Throw specific exceptions for errors.

Corresponding KeePassXC side PR: keepassxreboot/keepassxc#10318

@droidmonkey
Copy link
Member

Need to also add an https/certificate validity check if that is possible. See #2122

keepassxc-browser/background/client.js Outdated Show resolved Hide resolved
@varjolintu
Copy link
Member Author

varjolintu commented Feb 21, 2024

I cannot get the Microsoft Passkeys creation to work with Firefox (macOS). It always popups the OS' own Passkeys prompt instead. I thought it might be related to this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1878598 but after setting that option to false, I get a different prompt from Firefox.

I'm still trying to disable few other options too and see if it helps.

EDIT: Nope.

Copy link

@Ortham Ortham left a comment

Choose a reason for hiding this comment

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

Hi, I'm still going through the corresponding KeePassXC changes (I've just gotten through the create credentials flow, still have get to go), but I spotted some issues and some things I'm not sure about in this PR.

keepassxc-browser/content/passkeys.js Show resolved Hide resolved
keepassxc-browser/content/passkeys.js Show resolved Hide resolved
keepassxc-browser/content/passkeys-utils.js Outdated Show resolved Hide resolved
keepassxc-browser/content/passkeys-utils.js Show resolved Hide resolved
keepassxc-browser/content/passkeys-utils.js Show resolved Hide resolved
keepassxc-browser/content/keepassxc-browser.js Outdated Show resolved Hide resolved
keepassxc-browser/content/keepassxc-browser.js Outdated Show resolved Hide resolved
keepassxc-browser/content/passkeys-utils.js Outdated Show resolved Hide resolved
keepassxc-browser/content/passkeys-utils.js Outdated Show resolved Hide resolved
@varjolintu varjolintu force-pushed the fix/passkeys_improvements branch from bb0d98d to 6aa8c6f Compare February 29, 2024 16:11
Copy link

@Ortham Ortham left a comment

Choose a reason for hiding this comment

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

I just spotted one redundant if condition in your changes.

keepassxc-browser/content/passkeys-utils.js Outdated Show resolved Hide resolved
keepassxc-browser/content/passkeys.js Outdated Show resolved Hide resolved
keepassxc-browser/content/passkeys.js Outdated Show resolved Hide resolved
@varjolintu varjolintu force-pushed the fix/passkeys_improvements branch from df36cd0 to f6e2a86 Compare March 2, 2024 14:58
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

Adding a final approval, works great

@varjolintu varjolintu merged commit 87374df into develop Mar 4, 2024
@varjolintu varjolintu deleted the fix/passkeys_improvements branch March 4, 2024 16:11
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