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

Enhance Cloudinary CLI for Security and Optimization #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simran-sankhala
Copy link

Brief Summary of Changes

Addressed the potential security risk by implementing thorough input validation and sanitization for user-provided arguments and options. This helps mitigate the risk of code injection and ensures that unexpected behavior does not occur due to malicious input. Specifically, the code now validates and sanitizes user input before processing.

What does this PR address?

  • GitHub issue (Add reference - #XX)
  • Refactoring
  • New feature
  • Bug fix
  • Adds more tests

Are tests included?

  • Yes
  • No

Addressed the potential security risk by implementing thorough input validation and sanitization for user-provided arguments and options. This helps mitigate the risk of code injection and ensures that unexpected behavior does not occur due to malicious input. Specifically, the code now validates and sanitizes user input before processing.
Copy link
Contributor

@const-cloudinary const-cloudinary left a comment

Choose a reason for hiding this comment

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

@simran-sankhala, first of all thank you for contribution!
I like your refactoring, it makes code much more organized and easier to read!

Regarding input validation, I just saw the check that user indeed specified a valid directory, which is a good thing to check!
Are there any other things that are changed that I am missing?

Could you also add 1-2 tests covering this use case?

BTW, tests failed. Can you please check that?

See my other comments.

@click.option("-o", "--optional_parameter", multiple=True, nargs=2, help="Pass optional parameters as raw strings.")
@click.option("-O", "--optional_parameter_parsed", multiple=True, nargs=2, help="Pass optional parameters as interpreted strings.")
@click.option("-t", "--transformation", help="The transformation to apply on all uploads.")
@click.option("-f", "--folder", default="", help="The path where you want to upload the assets. The path you specify will be pre-pended to the public IDs of the uploaded assets. You can specify a whole path, for example path1/path2/path3. Any folders that do not exist are automatically created.")
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, we keep line length up to 120 chars.

**upload_dir_options,
**group_params(optional_parameter, ((k, parse_option_value(v)) for k, v in optional_parameter_parsed)),
}
options = prepare_upload_options(transformation, preset, optional_parameter, optional_parameter_parsed, folder, folder_mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Like! it makes code much more readable!

@simran-sankhala
Copy link
Author

sure , let me work on that test cases

@colbyfayock
Copy link

@simran-sankhala do you plan on continuing to work on this?

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