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

Set default algorithms for pubKeyCredParams #646

Closed
wants to merge 0 commits into from

Conversation

Jampire
Copy link

@Jampire Jampire commented Sep 8, 2024

Target branch:

  • It is a Bug fix
  • It is a New feature
  • Breaks BC
  • Includes Deprecations

PublicKeyCredentialCreationOptions can be created with an empty pubKeyCredParams array by default. However, some of the authenticators (like NordPass) don't support this. They are crashed when no algorithms are provided for registration options. Other authenticators (like 1Password) use ES256 (-7) by default on their side in such case.

@simplewebauthn/server sets [-8, -7, -257] algorithms by default. The same is done by W3C Webauthn specification.

This PR suggests to do the same.

Also, there is incorrect information in the documentation:

An empty list corresponds to the default algorithms that are ES256 and RS256 (in this order).

It's not true right now: an empty array is sent to the authenticator.

@Spomky
Copy link
Contributor

Spomky commented Sep 9, 2024

Related to w3c/webauthn#1757

@Spomky Spomky self-assigned this Sep 9, 2024
@Spomky Spomky added DX Developer Experience compliance Issue related to the compliance of the project with the specs UX labels Sep 9, 2024
@Spomky Spomky added this to the 5.0.2 milestone Sep 9, 2024
@Spomky Spomky changed the base branch from 5.1.x to 5.0.x September 9, 2024 06:27
@Spomky Spomky changed the base branch from 5.0.x to 5.1.x September 9, 2024 06:27
@Spomky
Copy link
Contributor

Spomky commented Sep 9, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Sep 9, 2024

rebase

☑️ Nothing to do

  • any of:
    • #commits-behind>0 [📌 rebase requirement]
    • #commits>1 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • -conflict [📌 rebase requirement]
  • queue-position=-1 [📌 rebase requirement]

@Spomky Spomky changed the base branch from 5.1.x to 5.0.x September 9, 2024 06:28
@Spomky
Copy link
Contributor

Spomky commented Sep 9, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Sep 9, 2024

rebase

✅ Branch has been successfully rebased

@Spomky Spomky force-pushed the default_algorithms branch from 4868f92 to 72a7b7c Compare September 9, 2024 06:29
@Spomky Spomky changed the base branch from 5.0.x to 5.1.x September 9, 2024 06:30
@Spomky
Copy link
Contributor

Spomky commented Sep 9, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Sep 9, 2024

rebase

❌ Base branch update has failed

Git reported the following error:

warning: skipped previously applied commit 6e0a4e9
warning: skipped previously applied commit d17d55f
warning: skipped previously applied commit f4ad425
warning: skipped previously applied commit 9983749
warning: skipped previously applied commit 4a09fd8
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
Rebasing (1/2)
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git rebase --skip'
interactive rebase in progress; onto 99f3d12
Last command done (1 command done):
   pick e566266 Update matthiasnoback/symfony-dependency-injection-test requirement || ^6.0
Next command to do (1 remaining command):
   pick 72a7b7c Set default algorithms for pubKeyCredParams
  (use "git rebase --edit-todo" to view and edit)
You are currently rebasing branch 'default_algorithms' on '99f3d12'.
  (all conflicts fixed: run "git rebase --continue")

nothing to commit, working tree clean
Could not apply e566266... Update matthiasnoback/symfony-dependency-injection-test requirement || ^6.0

@Spomky
Copy link
Contributor

Spomky commented Sep 9, 2024

Hi,

I tried to rebase it and merge into 5.0.x, but it looks like I do not have the permissions.
I created #647 that is exactly the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compliance Issue related to the compliance of the project with the specs DX Developer Experience UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants