parkpow / deep-license-plate-recognition

Automatic License Plate Recognition (ALPR) or Automatic Number Plate Recognition (ANPR) software that works with any camera.
https://platerecognizer.com/
MIT License
523 stars 122 forks source link

feat: send webhooks to Salient CompleteView events #179

Closed danleyb2 closed 7 months ago

danleyb2 commented 8 months ago

https://trello.com/c/r4gqkBUA/1307-salient-vms-integration

marcbelmont commented 7 months ago

@danleyb2 Don't forget to work on this. Also, let's see how the GPT reviewer does on this.

github-actions[bot] commented 7 months ago

Risk Level 3 - /home/runner/work/deep-license-plate-recognition/deep-license-plate-recognition/webhooks/webhook_salient/main.py

The code changes introduce several potential issues that should be addressed:

  1. Hardcoded server port: The server is set to run on port 8001 without an option to change it via environment variables or command-line arguments. It's better to allow configuration of the server port.
    port = int(os.environ.get('PORT', 8001))
    server = HTTPServer(('', port), handler)
  2. Disabling SSL verification: The line urllib3.disable_warnings() along with verify=False in the requests.post call disables SSL certificate verification, which can lead to security vulnerabilities. If this is for a development environment, it should be configurable and not set by default for production.
  3. Exception handling in notify_salient: The exception handling prints errors but does not re-raise them or handle them in a way that the caller can react to the failure. This could lead to silent failures in the notification system.
  4. Potential for KeyError: The code assumes that data['camera_id'] and data['timestamp'] will always be present in json_data. If these keys are missing, a KeyError will be raised. It's safer to use .get() with a default value.
  5. Content-Length parsing without validation: The code reads the Content-Length header and converts it to an integer without validation. This could lead to a ValueError if the header is not a valid integer.
    try:
       content_length = int(self.headers['Content-Length'])
    except ValueError:
       self.send_response(400)
       self.end_headers()
       return

🔧🔒🐛


Powered by Code Review GPT

danleyb2 commented 7 months ago

@adolfoarmas regarding the folder structure, The larger idea is to also add OpenEye and Synology as a destination in the forwarder. This is because they are all basically a server that format the webhook payload then call and external API

ttayson commented 7 months ago

@marcbelmont @danleyb2 I have only one point to highlight: I suggest the creation of bookmarks instead of events, along with the use of the latest NX API that does not require Digest authentication. The use of bookmarks makes the boards searchable in the system, in addition to providing a newer and recommended authentication method.

We have two NX implementations in our guides. The implementation below utilizes bookmarks: https://github.com/parkpow/deep-license-plate-recognition/tree/master/webhooks/Webhook_nx

Is there any limitation for using bookmarks that made us choose events instead?

adolfoarmas commented 7 months ago

@adolfoarmas regarding the folder structure, The larger idea is to also add OpenEye and Synology as a destination in the forwarder. This is because they are all basically a server that format the webhook payload then call and external API

Ok, it until they can be fully integrated in the forwarded, i think it is better to have all those VMS integrations grouped. The folders names doesn't necessarily need to be the one that i suggested.

Your thoughts @danleyb2 @ttayson @marcbelmont

danleyb2 commented 7 months ago

@ttayson @marcbelmont @adolfoarmas Suggestions:

Question: How should we handle support for different NX VMS versions because the endpoint changes? It's easier and recommended to support latest only then advise users to upgrade

Based on NX API versions and their support from here If the integration intends to support VMS 5.0+, it must prefer /rest/v1 which was introduced in this VMS version. If it intends to support VMS 5.1+, it must prefer /rest/v2, and so on. Originally (up to and including VMS 4.2), there was a mostly procedural-style API, with URLs mostly starting with /api/ and /ec2/ - these functions are now referred to as a "LEGACY & BETA" API. Latest is /rest/v3