Skip to content

Passkey improvements#2121

Merged
varjolintu merged 2 commits intodevelopfrom
fix/passkeys_improvements
Mar 4, 2024
Merged

Passkey improvements#2121
varjolintu merged 2 commits intodevelopfrom
fix/passkeys_improvements

Conversation

@varjolintu
Copy link
Copy Markdown
Member

@varjolintu varjolintu commented Feb 19, 2024 •
edited by droidmonkey
Loading

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

@varjolintu varjolintu added this to the Next milestone Feb 19, 2024
@varjolintu varjolintu mentioned this pull request Feb 19, 2024
Merged
@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Feb 19, 2024

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

droidmonkey
droidmonkey approved these changes Feb 19, 2024
View reviewed changes
@varjolintu
Copy link
Copy Markdown
Member Author

varjolintu commented Feb 21, 2024 •
edited
Loading

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.

@varjolintu varjolintu mentioned this pull request Feb 22, 2024
Closed
@droidmonkey droidmonkey mentioned this pull request Feb 22, 2024
Closed
@varjolintu varjolintu mentioned this pull request Feb 25, 2024
Closed
@varjolintu varjolintu force-pushed the fix/passkeys_improvements branch from c6c056e to f6ea29d Compare February 25, 2024 12:23
@varjolintu varjolintu modified the milestones: Next, 1.9.0 Feb 25, 2024
Ortham
Ortham suggested changes Feb 26, 2024
View reviewed changes
Copy link
Copy Markdown

@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.

@varjolintu varjolintu force-pushed the fix/passkeys_improvements branch from bb0d98d to 6aa8c6f Compare February 29, 2024 16:11
Ortham
Ortham reviewed Mar 1, 2024
View reviewed changes
Copy link
Copy Markdown

@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.

Ortham
Ortham suggested changes Mar 2, 2024
View reviewed changes
@varjolintu varjolintu force-pushed the fix/passkeys_improvements branch from df36cd0 to f6e2a86 Compare March 2, 2024 14:58
droidmonkey
droidmonkey approved these changes Mar 3, 2024
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

@phoerious phoerious Awaiting requested review from phoerious

+1 more reviewer

@Ortham Ortham Ortham requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

passkeys refactor

Projects

None yet

Milestone

1.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@varjolintu @droidmonkey @Ortham