Array.prototype methods cause validation error in the expected transp…

[thirug010]

added condition to skip Array.Prototype method irritated as item/object in the array for issue "https://github.com/webauthn-open-source/fido2-lib/issues/123"

[codecov-commenter]

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.06 :warning:

Comparison is base (aef3754) 92.95% compared to head (1af4825) 92.89%.

:exclamation: Current head 1af4825 differs from pull request most recent head f634222. Consider uploading reports for the commit f634222 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #125 +/- ## ========================================== - Coverage 92.95% 92.89% -0.06% ========================================== Files 16 16 Lines 5992 5998 +6 ========================================== + Hits 5570 5572 +2 - Misses 422 426 +4 ``` | [Impacted Files](https://codecov.io/gh/webauthn-open-source/fido2-lib/pull/125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webauthn-open-source) | Coverage Δ | | |---|---|---| | [lib/parser.js](https://codecov.io/gh/webauthn-open-source/fido2-lib/pull/125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webauthn-open-source#diff-bGliL3BhcnNlci5qcw==) | `88.55% <33.33%> (-0.42%)` | :arrow_down: | | [lib/validator.js](https://codecov.io/gh/webauthn-open-source/fido2-lib/pull/125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webauthn-open-source#diff-bGliL3ZhbGlkYXRvci5qcw==) | `92.06% <33.33%> (-0.24%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webauthn-open-source). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webauthn-open-source)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[JamesCullum]

Thanks a lot for your contribution - can you give an example of what you were trying before that didn't work, but works now with your code?

Maybe you can add this as test, so that we can future-proof it?

[thirug010]

Thanks a lot for your contribution - can you give an example of what you were trying before that didn't work, but works now with your code?

Maybe you can add this as test, so that we can future-proof it?

For my nodejs app we have few prototype function for ease of use like contains(), pushRange() ..., because of usage of 'in' to iterate the elements of array it consider the methods also as elements to iterate and fails the validation such as transports is not type of string.

Example: Array.prototype.contains =function(str) {.....} , Array.prototype.pushRange =function(items) {} var tempArray = ['usb','other','internal'] when array has two above prototype methods added, for(let a in tempArray ) show total of 5 item including the 3 elements and 2 porotype methods.

I will add the test cases for the same.

[wparad]

This is confusing and adds unnecessary bloat to the library. Adding things here to account for individual use cases that are incorrectly implemented decreases the value being provided by the library.

I'm going to recommend that if you create the poly fills appropriately to exclude the functions from the array iterator in your implementation code, as it does not belong here.

[thirug010]

This is confusing and adds unnecessary bloat to the library. Adding things here to account for individual use cases that are incorrectly implemented decreases the value being provided by the library.

I'm going to recommend that if you create the poly fills appropriately to exclude the functions from the array iterator in your implementation code, as it does not belong here.

ideally for..of need to be used to iterates over the values of an iterate-able object. instead of for..in iterates over all enumerable property keys of an object.

for..of is used in almost of the place to iterate object of array in the code, expect while iterating through allowCredentials, transports only.

using for..of to iterate allowCredentials, transports cause more test cases to fail, so I gone with for..in with filtering out the methods.

I believe array.protyotype methods are very common to use, so I thought adding this would help.

[JamesCullum]

I agree to some extent with both of you. If "for of" is the correct syntax, I think one needs to look closer at why the other tests are failing after a change - this would imply that either "for in" is the better choice or there are some hard wired dependencies.

I'm not really sure about your fix though - you are just checking if it is a function, and then skipping most validations etc. This might be dangerous to users who are accidently passing a function and are relying on everything to work properly.

My understanding of the use case would be that if it is a function, it should execute it and validate the result as if it was a value - is my understanding correct?

And coverage shouldn't decrease, so we'd always need tests for new things.

[thirug010]

"I'm not really sure about your fix though - you are just checking if it is a function, and then skipping most validations etc. This might be dangerous to users who are accidently passing a function and are relying on everything to work properly"

==> Yes I agree, this is not correct, I am checking the code / test cases so this issue can be fixed fully with for...of

"My understanding of the use case would be that if it is a function, it should execute it and validate the result as if it was a value - is my understanding correct?" ==> Yes

"And coverage shouldn't decrease, so we'd always need tests for new things" ==> we need to adjust / update the current test case (or code) if need and need to add test cases to cover for..of with method