webauthn-open-source / fido2-lib

A node.js library for performing FIDO 2.0 / WebAuthn server functionality
https://webauthn.io
MIT License
394 stars 118 forks source link

Add missing certification extension type LocalIntegerValueBlock #156

Closed wparad closed 5 months ago

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ad1566e) 92.86% compared to head (850d291) 92.86%.

:exclamation: Current head 850d291 differs from pull request most recent head 8a3ad1a. Consider uploading reports for the commit 8a3ad1a to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #156 +/- ## ======================================= Coverage 92.86% 92.86% ======================================= Files 16 16 Lines 6023 6027 +4 ======================================= + Hits 5593 5597 +4 Misses 430 430 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JamesCullum commented 5 months ago

Thanks a lot for the PR, highly appreciated!

Can you clarify why you moved the validation into the try-catch? When would one encounter the LocalIntegerValueBlock?

Please revert the changes in the dist/ folder - this one is not really meant to be manually edited.

wparad commented 5 months ago

Can you clarify why you moved the validation into the try-catch? When would one encounter the LocalIntegerValueBlock?

The unit test contains a cert with this exact example. This error is directly from one of our users, so it's a real cert. It looks like it is some sort of you ikey, I don't have any more information than that.

The validation isn't in the try catch, just resolving non critical extensions. If an extension isn't critical and we can't decode the data then there is no reason to throw an Error.

What we really want to do here is always prevent Errors on new data types, since we never want a production error based on a non-critical extension. So of course the parsing of the extension data belongs inside the try catch as well.

Re: the dist directory.

Why is this even committed in the repository, can we just git ignore the whole directory then?

JamesCullum commented 5 months ago

Agreed, thanks for the clarification!

Why is this even committed in the repository, can we just git ignore the whole directory then?

We need this to provide an authentic file to the CDN, especially for Deno