Ensure non-ambiguous args/env vars injection into PEXes #18861
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaos
approved these changes
Apr 30, 2023
jsirois
approved these changes
Apr 30, 2023
|
Ah, good catch. There's two issues:
|
huonw
added a commit
to huonw/pants
that referenced
this pull request
May 1, 2023
…8861) This fixes pantsbuild#18779 by ensuring that injecting arguments and environment variables into a pex doesn't risk them being interpreted by the pex CLI itself if they start with `-`, and ensuring they're quoted appropriately to match the `shlex.split` within pex (https://github.com/pantsbuild/pex/blob/ff220b9e41484450e0c78136f98d6899f2e2325c/pex/bin/pex.py#L433). The issue noted in pantsbuild#18779 was specifically ambiguity with `args`, but the same problem will apply to `env` and so that is fixed here too (although it's rather less common to have env vars starting with `-`). NB. `shlex.quote`ing alone won't handle the `-` ambiguity, `shlex.quote("--foo") == "--foo"`. For a target like `pex_binary(..., args=["--foo"], env={"--bar": "baz"})`: - Previously, this would invoke `pex ... --inject-args --foo --inject-env --bar=baz`, and argparse within the pex CLI will intepret `--foo` and `--bar=...` as independent parameters, rather than arguments to `--inject-args`/`--inject-env`. - After this change (as suggested by @jsirois), the invocation will be `pex ... --inject-args=--foo --inject-env=--bar=baz` and there's no risk of misinterpretation. For a target like `pex_binary(..., args=["spaces 'n' quotes"])`: - Previously this would invoke with (equivalently) an arg `--inject-args=spaces 'n' quotes`, which would `shlex.split` to three arguments in the PEX, the same as passing `args=["spaces", "n", "quotes"])` - Now, this will invoke with `--inject-args='spaces '"'"'n'"'"' quotes'`, which, is correctly `shlex.split` to the single value: `spaces 'n' quotes`.
huonw
added a commit
that referenced
this pull request
May 2, 2023
#18861) (#18877) This fixes #18779 by ensuring that injecting arguments and environment variables into a pex doesn't risk them being interpreted by the pex CLI itself if they start with `-`, and ensuring they're quoted appropriately to match the `shlex.split` within pex (https://github.com/pantsbuild/pex/blob/ff220b9e41484450e0c78136f98d6899f2e2325c/pex/bin/pex.py#L433). The issue noted in #18779 was specifically ambiguity with `args`, but the same problem will apply to `env` and so that is fixed here too (although it's rather less common to have env vars starting with `-`). NB. `shlex.quote`ing alone won't handle the `-` ambiguity, `shlex.quote("--foo") == "--foo"`. For a target like `pex_binary(..., args=["--foo"], env={"--bar": "baz"})`: - Previously, this would invoke `pex ... --inject-args --foo --inject-env --bar=baz`, and argparse within the pex CLI will intepret `--foo` and `--bar=...` as independent parameters, rather than arguments to `--inject-args`/`--inject-env`. - After this change (as suggested by @jsirois), the invocation will be `pex ... --inject-args=--foo --inject-env=--bar=baz` and there's no risk of misinterpretation. For a target like `pex_binary(..., args=["spaces 'n' quotes"])`: - Previously this would invoke with (equivalently) an arg `--inject-args=spaces 'n' quotes`, which would `shlex.split` to three arguments in the PEX, the same as passing `args=["spaces", "n", "quotes"])` - Now, this will invoke with `--inject-args='spaces '"'"'n'"'"' quotes'`, which, is correctly `shlex.split` to the single value: `spaces 'n' quotes`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #18779 by ensuring that injecting arguments and environment variables into a pex doesn't risk them being interpreted by the pex CLI itself if they start with
-, and ensuring they're quoted appropriately to match theshlex.splitwithin pex (https://github.com/pantsbuild/pex/blob/ff220b9e41484450e0c78136f98d6899f2e2325c/pex/bin/pex.py#L433).The issue noted in #18779 was specifically ambiguity with
args, but the same problem will apply toenvand so that is fixed here too (although it's rather less common to have env vars starting with-). NB.shlex.quoteing alone won't handle the-ambiguity,shlex.quote("--foo") == "--foo".For a target like
pex_binary(..., args=["--foo"], env={"--bar": "baz"}):pex ... --inject-args --foo --inject-env --bar=baz, and argparse within the pex CLI will intepret--fooand--bar=...as independent parameters, rather than arguments to--inject-args/--inject-env.pex ... --inject-args=--foo --inject-env=--bar=bazand there's no risk of misinterpretation.For a target like
pex_binary(..., args=["spaces 'n' quotes"]):--inject-args=spaces 'n' quotes, which wouldshlex.splitto three arguments in the PEX, the same as passingargs=["spaces", "n", "quotes"])--inject-args='spaces '"'"'n'"'"' quotes', which, is correctlyshlex.splitto the single value:spaces 'n' quotes.