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

Add missing certification extension type LocalIntegerValueBlock #156

Merged
merged 2 commits into from
Feb 10, 2024

Conversation

wparad
Copy link
Contributor

@wparad wparad commented Feb 5, 2024

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JamesCullum
Copy link
Member

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.

@JamesCullum JamesCullum self-requested a review February 6, 2024 22:15
@wparad
Copy link
Contributor Author

wparad commented Feb 7, 2024

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
Copy link
Member

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

JamesCullum
JamesCullum previously approved these changes Feb 10, 2024
dist/main.cjs Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is for deno, doesn't Deno just use npm like everything else?

Copy link
Member

@JamesCullum JamesCullum Feb 10, 2024

Choose a reason for hiding this comment

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

Deno can do that, but one of the big advantages is that it can simply reference internet-hosted files directly, if they are bundled. To allow that workflow, we bundle it in Git and Deno-users can directly use it without having to bundle it. You can find the example in the README

@JamesCullum JamesCullum merged commit a7d1bdf into webauthn-open-source:master Feb 10, 2024
12 checks passed
@wparad wparad deleted the patch-1 branch February 10, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants