[PM-6125] fix: handle cases when kp.alg is string in createCredential for…#7832
[PM-6125] fix: handle cases when kp.alg is string in createCredential for…#7832AlessioC31 wants to merge 1 commit intobitwarden:mainfrom
kp.alg is string in createCredential for…#7832Conversation
|
Thank you for your contribution! We've added this to our internal Community PR board for review. |
kp.alg is string in createCredential for…kp.alg is string in createCredential for…
|
No New Or Fixed Issues Found |
| credTypesAndPubKeyAlgs = params.pubKeyCredParams.filter( | ||
| (kp) => kp.alg === -7 && kp.type === "public-key", | ||
| ); | ||
| credTypesAndPubKeyAlgs = params.pubKeyCredParams.filter((kp) => { |
There was a problem hiding this comment.
issue: could you add a test case to check that this works? (we want Fido2Client well-tested)
| (kp) => kp.alg === -7 && kp.type === "public-key", | ||
| ); | ||
| credTypesAndPubKeyAlgs = params.pubKeyCredParams.filter((kp) => { | ||
| if (typeof kp.alg == "string") { |
There was a problem hiding this comment.
issue: we usually use strict equality wherever possible
| if (typeof kp.alg == "string") { | |
| if (typeof kp.alg === "string") { |
| (kp) => kp.alg === -7 && kp.type === "public-key", | ||
| ); | ||
| credTypesAndPubKeyAlgs = params.pubKeyCredParams.filter((kp) => { | ||
| if (typeof kp.alg == "string") { |
There was a problem hiding this comment.
suggestion: Could you add a comment here (and above the test case) explaining that this is a fix for non-compliant websites?
| if (typeof kp.alg == "string") { | |
| // Fix for spec-deviation: Sites using KeycloakJS send `kp.alg` as a string | |
| if (typeof kp.alg == "string") { |
|
Hello @coroiu , thanks for the comments. I see that you created a new PR to address your comments, sorry but didn't have the time to reply these past days. Should I consider this as done then? |
|
@AlessioC31 No worries :) Normally we would've just waited but I'm going on vacation at the end of the day and wanted to get this merged before! Thanks for your contribution! Closing this in favor of #10285 |
|
hey @AlessioC31 - what website were you using that sent |
…
Fido2Type of change
Objective
I was trying to add passkeys from a website that uses a version of KeycloakJS that sends
kp.algas string.I know this deviates from standard but it would help passkeys adoption in Bitwarden while providers fix their code (and organizations install the new version).
With this PR,
createCredentialhandles passkeys creation whenkp.algisstringby parsing it as aNumber.Let me know if you think this should be handled differently, thanks!
Code changes
Parse
kp.algasNumberif it is astring.Fixes #6804 .