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(config/html): handle encoding issues and improve error handling in config and HTML file loading functions #4203

Merged
merged 12 commits into from
Jun 7, 2024

Conversation

lshw54
Copy link
Contributor

@lshw54 lshw54 commented Jun 7, 2024

Context

This pull request addresses issues related to file encoding and error handling in configuration file loading functions (load_and_validate_config_file and load_and_validate_fixer_config_file) and the HTML output generation function (fill_html_overview_statistics). The changes ensure that files are read and written with the correct encoding, preventing UnicodeDecodeError.

Description

  1. Encoding Handling:
  • Updated load_and_validate_config_file, load_and_validate_fixer_config_file, and fill_html_overview_statistics to open files with encoding='utf-8' to ensure they are read and written correctly as UTF-8 encoded text.

  • This change prevents UnicodeDecodeError caused by incorrect or unsupported file encodings.

  1. Specific Exception Handling:
  • Added specific exception handling for FileNotFoundError, yaml.YAMLError, and UnicodeDecodeError in the configuration file loading functions.
  • Added specific exception handling for FileNotFoundError and UnicodeDecodeError in the HTML output generation function.
  • This provides clearer error messages and makes debugging easier.
  1. Default Values:
  • Used default values in stats.get() in fill_html_overview_statistics to handle cases where keys might be missing.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lshw54 lshw54 requested review from a team as code owners June 7, 2024 07:47
except Exception as e:
logger.critical(f"{e.__class__.__name__}[{e.__traceback__.tb_lineno}] -- {e}")

sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

I think the sys.exit(1) should happen inside each exception right?

except Exception as e:
logger.critical(f"{e.__class__.__name__}[{e.__traceback__.tb_lineno}] -- {e}")

sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@pedrooot pedrooot left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! @lshw54 Could you please review my comments? 🚀

@pedrooot pedrooot self-assigned this Jun 7, 2024
@lshw54
Copy link
Contributor Author

lshw54 commented Jun 7, 2024

Thanks for the improvements! @lshw54 Could you please review my comments? 🚀

Thanks!!I have made the changes to move sys.exit(1) inside each specific exception block in load_and_validate_config_file, load_and_validate_fixer_config_file, and fill_html_overview_statistics.

@pedrooot
Copy link
Member

pedrooot commented Jun 7, 2024

Could you have a look to the tests? It seems that flake8 it's failing. Using pre-commit should be enough https://docs.prowler.com/projects/prowler-open-source/en/latest/developer-guide/introduction/

@lshw54
Copy link
Contributor Author

lshw54 commented Jun 7, 2024

Could you have a look to the tests? It seems that flake8 it's failing. Using pre-commit should be enough https://docs.prowler.com/projects/prowler-open-source/en/latest/developer-guide/introduction/

All pre-commit run result is passed

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 32.14286% with 19 lines in your changes missing coverage. Please review.

Project coverage is 86.61%. Comparing base (28b9e26) to head (3e70fd5).
Report is 6 commits behind head on master.

Files Patch % Lines
prowler/config/config.py 47.36% 10 Missing ⚠️
prowler/lib/outputs/html/html.py 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4203      +/-   ##
==========================================
- Coverage   86.66%   86.61%   -0.06%     
==========================================
  Files         818      818              
  Lines       25674    25691      +17     
==========================================
+ Hits        22251    22252       +1     
- Misses       3423     3439      +16     

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

@lshw54
Copy link
Contributor Author

lshw54 commented Jun 7, 2024

Hi @pedrooot Thanks for your comment, it seems that I added some new comments and optimized a small portion of the code which resulted in a drop in coverage.

@pedrooot
Copy link
Member

pedrooot commented Jun 7, 2024

True, don't worry about that. Btw we are reviewing it now 😄

Copy link
Member

@sergargar sergargar left a comment

Choose a reason for hiding this comment

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

Hi @lshw54, thanks for this fix!
Could you please change the log level to error and not exiting in those new errors? I don't think it could be a reason to exit Prowler execution since the scan can still continue.

@lshw54
Copy link
Contributor Author

lshw54 commented Jun 7, 2024

Could you please change the log level to error and not exiting in those new errors? I don't think it could be a reason to exit Prowler execution since the scan can still continue.

I have made the requested changes. The log level has been changed to error for the specific exceptions, and the program will no longer exit. This allows the scan to continue even if these errors occur. Errors are now logged at the error level, and the function allows the scan to proceed without exiting the Prowler.

@sergargar sergargar self-requested a review June 7, 2024 15:20
prowler/config/config.py Outdated Show resolved Hide resolved
prowler/lib/outputs/html/html.py Outdated Show resolved Hide resolved
prowler/config/config.py Outdated Show resolved Hide resolved
prowler/config/config.py Outdated Show resolved Hide resolved
@sergargar sergargar self-requested a review June 7, 2024 16:20
@sergargar
Copy link
Member

Could you please change the log level to error and not exiting in those new errors? I don't think it could be a reason to exit Prowler execution since the scan can still continue.

I have made the requested changes. The log level has been changed to error for the specific exceptions, and the program will no longer exit. This allows the scan to continue even if these errors occur. Errors are now logged at the error level, and the function allows the scan to proceed without exiting the Prowler.

Thanks for the changes @lshw54 ! I also change the error messages to follow our pattern.

@sergargar sergargar merged commit e28300a into prowler-cloud:master Jun 7, 2024
9 of 11 checks passed
sergargar added a commit that referenced this pull request Jun 7, 2024
…n config and HTML file loading functions (#4203)

Co-authored-by: Sergio <sergio@prowler.com>
sergargar added a commit that referenced this pull request Jun 7, 2024
…n config and HTML file loading functions (#4203)

Co-authored-by: Sergio <sergio@prowler.com>
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.

None yet

3 participants