search

agsh / onvif

ONVIF node.js implementation
http://agsh.github.io/onvif/
MIT License
692 stars 234 forks source link

(too) optimistic number parsing #217

Closed mhahn-sighthoundlabs closed 1 year ago

mhahn-sighthoundlabs commented 2 years ago

When converting to what-looks-like-a-number in

https://github.com/agsh/onvif/blob/a218a6b836b5088f7c953cca6a3835d0a99ab287/lib/utils.js#L36

it can scramble strings which are not numbers, e.g. Wiegand (binary) codes represented like "101010100110011010", beyond 56bit length. Those then turn into a numbers which are lossy i.e. cannot restored to their original form.

One solution would be to check if the expression overflows the number space and then leave it as a string. Or have some custom way to work around such edge cases.

agsh commented 2 years ago

@mhahn-sighthoundlabs Hi! I agree with you. Maybe I should use something like this: https://mathjs.org/docs/datatypes/fractions.html ? What do you think?

mhahn-sighthoundlabs commented 2 years ago

It's tricky. Interpreting any data type out of a certain pattern might lead to collisions.

We first try just to patch the float-parse step, but that wasn't enough, because our problematic material sometimes had had leading zeros and was then still consumed the wrong way. So we added a (very dirty) hook to just detect it and exclude it from any interpretation, i.e. let it be passed a string. This hack can be found here:

https://github.com/agsh/onvif/compare/master...sighthoundinc:fix/issue-217

Since this might happen again for other people with other colliding material, I can only think about either just leaving everything as a string (via different mode, to remain backwards compatible in the API).

Or attach the original string,m for dates and numbers with another property (like __value), so in the worst case people have access to the original XML material. Bloats the objects a bit though.

agsh commented 2 years ago

@mhahn-sighthoundlabs Hmmm... Interesting fix, I can add this to the master, if you make a pull request. And further to think about string holding for floating numbers. Thank you!

RogerHardiman commented 2 years ago

I see you mentioned Weigand so I assume you are doing an access control system. Have you added some code for additional ONVIF services we can include in the library? Roger

mhahn-sighthoundlabs commented 2 years ago

@mhahn-sighthoundlabs Hmmm... Interesting fix, I can add this to the master, if you make a pull request. And further to think about string holding for floating numbers. Thank you!

I'll need to refactor this a bit to make it less global, stay tuned.

mhahn-sighthoundlabs commented 2 years ago

I see you mentioned Weigand so I assume you are doing an access control system. Have you added some code for additional ONVIF services we can include in the library? Roger

Sadly no. The access control system treated like a camera, or that's how we get access to the events it emits.