Skip to content

reporting: fix incorrect converting of list based commands#919

Merged
pirat89 merged 2 commits into
oamg:mainfrom
PeterMocary:fix-command-to-string
Mar 31, 2026
Merged

reporting: fix incorrect converting of list based commands#919
pirat89 merged 2 commits into
oamg:mainfrom
PeterMocary:fix-command-to-string

Conversation

@PeterMocary

@PeterMocary PeterMocary commented Mar 17, 2026

Copy link
Copy Markdown
Member

The reporting module handled conversion between shell command representations for the text and json reports incorrectly. The list of strings representation was converted without preserving the argument separation correctly which results in malformed commands in the text report.

This patch utilizes the built-in shlex Python library for the conversion which preserves the argument separation by wrapping arguments in quotes when necessary.

See the Jira for a lengthy description with examples.

Jira: RHEL-156521

NOTE: The shlex.join is available from Python 3.8+ as the failing tests show. However, the shlex.quote is available from Python 3.3 and the join function uses quote function anyway so we can just replicate the implementation of shlex.join in here to achieve the same.

@github-actions

Copy link
Copy Markdown

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp-repository*main* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp-repository*PR42* as artifacts
  • /rerun-sst to schedule sst tests using this pr build and leapp-repository*main* as artifacts
  • /rerun-sst 42 to schedule sst tests using this pr build and leapp-repository*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

@pirat89 pirat89 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add please also tests as well.

also do not forget to bump the leapp framework version in spec file.

The reporting module handled conversion between shell command
representations for the text and json reports incorrectly. The list of
strings representation was converted without preserving the argument
separation correctly which results in malformed commands in the text
report.

This patch utilizes the built-in shlex Python library for the conversion
which preserves the argument separation by wrapping arguments in quotes
when necessary.

Jira: RHEL-156521
@PeterMocary PeterMocary force-pushed the fix-command-to-string branch from 4149eeb to e8ac23d Compare March 20, 2026 18:36
@PeterMocary

PeterMocary commented Mar 20, 2026

Copy link
Copy Markdown
Member Author

Added tests, and bumped framework version in spec file.

I'd like to point out that this implementation has a flaw that stems from the implementation of the shlex.quote. It quotes the strings in ' which makes the arguments literals as expected, however, if there is ' in the argument the escaping of this character is quite ugly although correct.

print(shlex.quote("It's fine."))
'It'"'"'s fine.'

A possible solution to this would be to reimplement shlex.quote in a way that would use " quoting or alternatively the " quoting could be used only if ' is used in the argument string (basically replacing only the arguments that would result in the ugly single quote escaping the shlex.quote does), but that still requires the same implementation of escaping special characters. Implementing the escaping is in my opinion quite error prone so I chose not to implement it yet, namely because different shells could have different rules for escaping (! might be a problematic character for example). If we chose to go forward with the shlex, I will have to add some explanation into the documentation as well.

@PeterMocary

PeterMocary commented Mar 25, 2026

Copy link
Copy Markdown
Member Author

Added a custom implementation of the quoting that simply handles the cases where ' is in the string and quotes the string in " while making sure it is still a literal by escaping the characters with special meanings. The tests should showcase the quoting well enough. This implementation is bit more error prone (if the escaping fails to handle some corner case), but provides pretty human readable commands for the text version of report.

Added this so that we can chose between two concrete implementations, which will hopefully make it easier.

Note: It is also worth going through the influenced cases in the leapp-repository PR to have better idea of what we really use.

Comment thread leapp/reporting/__init__.py
Comment thread leapp/reporting/__init__.py Outdated
@PeterMocary

PeterMocary commented Mar 30, 2026

Copy link
Copy Markdown
Member Author

We should also decide whether to expose the current _quote_for_shell functionality so that when creating the command list one can use it to quote the underlying command layers (e.g. oamg/leapp-repository#1510 (comment) where the user's paths need to be quoted for the command wrapped in bash -c). The name could be changed to quote_command_arg or something similar in case it is exposed. If exposed, however, it might happen that people will use it on the command arguments in the list and effectively double quote them because leapp does it already. The function should be used outside of the framework only if the created command list has underlying command executed in another context same as the bash -c example.

@PeterMocary

Copy link
Copy Markdown
Member Author

/packit copr-build

@pirat89

pirat89 commented Mar 31, 2026

Copy link
Copy Markdown
Member

nice solution with a test coverage. I tried to find a broken example but I have not found any in the end.

@pirat89 pirat89 merged commit ad50bb2 into oamg:main Mar 31, 2026
24 checks passed
@PeterMocary PeterMocary added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants