Skip to content

Disable allow_abbrev from Python scripts using argparse #9294

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

juhaylinen
Copy link

@juhaylinen juhaylinen commented Jun 20, 2024

Description

Python's argparse library, by default, allows shortening of command line arguments. This can introduce silent failures when shortened commands are used and another command is added to the script which uses that name.

This change sets ArgumentParser parameter allow_abbrev to False.

PR checklist

  • changelog not required
  • 3.6 backport #9295
  • 2.28 backport #9296
  • tests not required

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Ok by me, but I've circulated this within the maintenance team in case some people want to.

For consistency, please make a companion pull request to https://github.com/Mbed-TLS/mbedtls-framework for the scripts in the framework submodule.

Also, if we decide this as a policy, we should add a note to the Python coding standards.

@gilles-peskine-arm gilles-peskine-arm added needs-design-approval needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review component-test Test framework and CI scripts priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) needs-backports Backports are missing or are pending review and approval. labels Jun 20, 2024
@davidhorstmann-arm davidhorstmann-arm self-requested a review June 20, 2024 12:35
@juhaylinen
Copy link
Author

Added backports to 3.6 and 2.28 and created PR Mbed-TLS/mbedtls-framework#29 to update the scripts in the framework submodule.

@gilles-peskine-arm
Copy link
Contributor

Please add a commit to update the framework submodule to the head of Mbed-TLS/mbedtls-framework#29. We'll ask you to update that commit again when the framework pull request is merged. We currently need this process to deal with the CI in the framework repository, we're still working on improving the automation.

@gilles-peskine-arm gilles-peskine-arm removed needs-design-approval needs-reviewer This PR needs someone to pick it up for review labels Jun 21, 2024
@gilles-peskine-arm
Copy link
Contributor

I'm removing needs-design-approval because I haven't heard any objections.

@@ -485,8 +485,9 @@ def write(self, filename=None):
def main():
"""Command line mbedtls_config.h manipulation tool."""
parser = argparse.ArgumentParser(description="""
Mbed TLS configuration file manipulation tool.
""")
Mbed TLS configuration file manipulation tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pylint is unhappy about the indentation.

To reproduce the CI failure:

scripts/min_requirements.py
tests/scripts/check-python-files.sh

You'll probably want to do this in a virtual environment with a Python version that isn't too recent, otherwise Pylint complains more. Just make sure

pylint scripts/config.py

doesn't complain about indentation anymore.

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 21, 2024
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Jun 24, 2024
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm 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 updates, looks good to me

@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jun 24, 2024
@juhaylinen
Copy link
Author

Hi @gilles-peskine-arm,
Is the anything I can do to move this forward?

@gilles-peskine-arm
Copy link
Contributor

Nothing, I'm afraid. We need to find review bandwidth and that's always a problem.

Python's argparse library, by default, allows shortening of command
line arguments. This can introduce silent failures when shortened
commands are used and another command is added to the script which
uses that name.

Signed-off-by: Juha Ylinen <[email protected]>
Fix indentation error in scripts/config.py

Signed-off-by: Juha Ylinen <[email protected]>
@juhaylinen
Copy link
Author

Rebased

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Mar 13, 2025
@gilles-peskine-arm
Copy link
Contributor

A lot of scripts have moved to https://github.com/Mbed-TLS/mbedtls-framework now. Those are unified between the development and 3.6 branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Test framework and CI scripts needs-backports Backports are missing or are pending review and approval. needs-work priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
Status: In Development
Development

Successfully merging this pull request may close these issues.

2 participants