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

fix-error-usernameNXintegration #187

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

ttayson
Copy link
Collaborator

@ttayson ttayson commented Mar 5, 2024

Note: Sorry for the typo in the branch name.

For some reason, when using a username argument on Windows, the user account name is captured, interfering with the information passed to the script.

Copy link

github-actions bot commented Mar 5, 2024

Risk Level 2 - /home/runner/work/deep-license-plate-recognition/deep-license-plate-recognition/webhooks/Webhook_nx/main.py

The changes include the addition of command-line arguments for login credentials and environment variable handling. The risk is moderate due to the potential exposure of sensitive information if not handled properly. However, there are no API keys or secrets directly exposed in the code changes.

  1. Environment Variable Fallbacks: It's good practice to provide a way to set configuration through environment variables as a fallback. This is implemented correctly for server_host, args.login, args.password, args.ssl, args.debug, args.tag, and args.parkpow_token.

  2. Error Handling: The addition of a check to ensure server_host, args.password, and args.login are not None is a good practice to avoid runtime errors due to misconfiguration.

  3. Security Concern: The verify parameter in the session.post and session.get calls is controlled by a command-line argument which can disable SSL verification. This could lead to security vulnerabilities if SSL verification is turned off in a production environment. It is recommended to enforce SSL verification or provide clear warnings when it is disabled.

  4. Potential Resource Leak: The use of Timer to create recurring tasks can lead to resource leaks if not managed properly. Ensure that timers are canceled and cleaned up when the application is terminated or when they are no longer needed.

  5. Password Handling: Storing passwords in plain text or passing them through command-line arguments can be insecure. Consider using a more secure method of handling credentials, such as environment variables with proper access controls or a secret management tool.

  6. Boolean Argument Parsing: The --ssl argument is parsed as a string but is expected to be a boolean. This could lead to unexpected behavior. Use action='store_true' or action='store_false' for boolean flags.

Example for boolean argument parsing:

parser.add_argument(\"--ssl\", action='store_true', help=\"Enable SSL verification\")
  1. Logging Sensitive Information: Be cautious about logging sensitive information such as passwords or tokens. Ensure that any logging does not expose this information in logs.

🔒🐛👀


Powered by Code Review GPT

@ttayson ttayson requested a review from danleyb2 March 5, 2024 23:34
@ttayson ttayson changed the title username correction fix-error-usernameNXintegratioon Mar 5, 2024
@ttayson ttayson changed the title fix-error-usernameNXintegratioon fix-error-usernameNXintegration Mar 5, 2024
@ttayson
Copy link
Collaborator Author

ttayson commented Mar 5, 2024

@adolfoarmas FYI

@adolfoarmas
Copy link
Collaborator

For some reason, when using a username argument on Windows, the user account name is captured, interfering with the information passed to the script.

@ttayson just curious, what value did you receive when "username" argument name was used? what a strange situation.

If it works now, LGTM!

Co-authored-by: Adolfo Armas <adolfoarmas.90@gmail.com>
@ttayson
Copy link
Collaborator Author

ttayson commented Mar 6, 2024

@adolfoarmas, in my case, I received "Talles Tayson" as the username from the login.

This happens because I use environment variables to capture the value when the user is using Docker, perhaps there is a better way to do this

@adolfoarmas
Copy link
Collaborator

@ttayson i see, there it is: https://adolfo-platerecognizer.tinytake.com/msc/OTM1NzU5OF8yMjkyNTQ1NQ

FYI https://docs.python.org/3/library/os.html#os.getenvb

os.getenv(key, default=None)
Return the value of the environment variable key as a string if it exists, or default if it doesn’t.

LGTM

@marcbelmont marcbelmont merged commit 2ed8db2 into master Mar 6, 2024
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