Repository navigation
fix(train): shell-quote values written to the training env file - #6517
Conversation
sm_training.env is consumed with `source`, so a value containing a single quote ended the quoting and the remainder ran as shell. A legitimate value such as "it's 0.001" was also silently truncated. Quote each value with shlex.quote.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI code review
This PR fixes a shell-injection / value-truncation bug in the generated sm_training.env file, which is consumed via source. It replaces the hand-rolled export KEY='value' quoting with shlex.quote(safe_serialize(value)) across all four relevant modules (sagemaker-train and sagemaker-core, both environment.py and bootstrap_runtime_environment.py), and adds bash-level tests that assert injection payloads round-trip verbatim and never execute. The change is correct, symmetric, well-tested, and consistent with established team precedent for shell-quoting user-controlled values (Source: #6356, Source: #6439). I found no issues; the EXPECTED_ENV test fixture was correctly updated to reflect that shlex.quote only adds quotes when needed (JSON values still quoted, simple tokens unquoted), and no downstream code parses the file expecting the old single-quote format.
Reviewed commit fac66d7. Automated review; verify before acting.
There was a problem hiding this comment.
🤖 AI code review
This PR replaces the naive export KEY='value' single-quote wrapping with shlex.quote(...) when writing sm_training.env, fixing a shell-injection / truncation bug for values containing single quotes or shell metacharacters. The change is applied symmetrically to both the sagemaker-core and sagemaker-train copies of environment.py and bootstrap_runtime_environment.py, and is backed by good tests that actually source the generated file under bash and assert the value round-trips verbatim without executing a marker payload. safe_serialize always returns a str, so shlex.quote receives a valid argument, and the consuming side uses source, so the fix is correct. Overall this is a clean, well-scoped security fix.
Reviewed commit e9c166c. Automated review; verify before acting.
| for key, value in env_vars.items(): | ||
| f.write(f"export {key}='{safe_serialize(value)}'\n") | ||
| # The file is consumed with `source`, so the value must be shell-quoted. | ||
| f.write(f"export {key}={shlex.quote(safe_serialize(value))}\n") |
There was a problem hiding this comment.
🟡 Low: This PR correctly fixes the value-quoting half of the sourcing problem, but not the non-identifier-variable-name half. Channel names are still written as export SM_CHANNEL_{channel.upper()}=... (line 183) without replacing -/. with _, unlike the hyperparameter keys at line 189. A channel named e.g. my-data would produce export SM_CHANNEL_MY-DATA=..., which fails to source as not a valid identifier. This is the companion issue to the value-escaping fix described in the prior work on this file. Source: #6439 . It's pre-existing and outside this PR's stated scope, but worth addressing together since both break source.
Issue #, if available:
Description of changes:
sm_training.env is consumed with
source, so a value containing a single quote ends the quotation and allows the rest of the value to run in the shell. A legitimate value such as "it's 0.001" was also silently truncated. Fix is to quote each value with shlex.quoteBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.