kada2004 commented on PR #69908:
URL: https://github.com/apache/airflow/pull/69908#issuecomment-5381000216
> This is exactly the right shape now — thanks for taking the bigger change
rather than patching the quoting. The passphrase never reaches the remote
command line, so there is nothing left to escape on either OS, and it is no
longer visible to anyone running `ps` on the remote host. The test with
`s3cr3t&rm -rf ~` is a good one to have: it fails loudly if the passphrase ever
creeps back onto the command string.
>
> One thing to restore before this lands, and one small note.
> ### `shlex.quote()` on the remote path was dropped
(`encryption_utils.py:54`)
>
> Before this PR the command built the input path as `-in
{shlex.quote(remote_enc_file)}`; it is now interpolated raw, and the `shlex`
import went with it. A remote path containing a space (a temp directory under a
user profile with a space in it, for example) will break the command, and it
puts an unquoted value back into a shell string for no benefit. This came from
the snippet in my previous review, which dropped the quoting — my mistake, not
something you should have caught. Re-adding `import shlex` and wrapping the
path restores the old behaviour.
> ### Smaller observations
>
> * `encryption_utils.py:61` — `password = None` under the comment
_"Clear password to prevent lingering sensitive data"_. Rebinding the local
name does not clear anything: the caller still holds the same string object,
and CPython gives no way to zero a `str` in place. This predates your change,
but it now sits inside the block you rewrote, and the comment promises a
guarantee the code cannot make. Dropping both lines would leave the function
honest — the real protection is the one you just added, which is keeping the
value off the command line.
>
>
> > _This review was drafted by an AI-assisted tool and
> > confirmed by an Airflow maintainer. The findings
> > below are observations, not blockers; an Airflow
> > maintainer — a real person — will take the next look at the
> > PR. If you think a finding is mis-applied, please reply on
> > the PR and a maintainer will weigh in._
> > _More on how Airflow handles maintainer review:_
> >
[contributing-docs/05_pull_requests.rst](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst).
>
> Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Thank you for catching this. Restored shlex.quote on remote path and drop
misleading password = None
> This is exactly the right shape now — thanks for taking the bigger change
rather than patching the quoting. The passphrase never reaches the remote
command line, so there is nothing left to escape on either OS, and it is no
longer visible to anyone running `ps` on the remote host. The test with
`s3cr3t&rm -rf ~` is a good one to have: it fails loudly if the passphrase ever
creeps back onto the command string.
>
> One thing to restore before this lands, and one small note.
> ### `shlex.quote()` on the remote path was dropped
(`encryption_utils.py:54`)
>
> Before this PR the command built the input path as `-in
{shlex.quote(remote_enc_file)}`; it is now interpolated raw, and the `shlex`
import went with it. A remote path containing a space (a temp directory under a
user profile with a space in it, for example) will break the command, and it
puts an unquoted value back into a shell string for no benefit. This came from
the snippet in my previous review, which dropped the quoting — my mistake, not
something you should have caught. Re-adding `import shlex` and wrapping the
path restores the old behaviour.
> ### Smaller observations
>
> * `encryption_utils.py:61` — `password = None` under the comment
_"Clear password to prevent lingering sensitive data"_. Rebinding the local
name does not clear anything: the caller still holds the same string object,
and CPython gives no way to zero a `str` in place. This predates your change,
but it now sits inside the block you rewrote, and the comment promises a
guarantee the code cannot make. Dropping both lines would leave the function
honest — the real protection is the one you just added, which is keeping the
value off the command line.
>
>
> > _This review was drafted by an AI-assisted tool and
> > confirmed by an Airflow maintainer. The findings
> > below are observations, not blockers; an Airflow
> > maintainer — a real person — will take the next look at the
> > PR. If you think a finding is mis-applied, please reply on
> > the PR and a maintainer will weigh in._
> > _More on how Airflow handles maintainer review:_
> >
[contributing-docs/05_pull_requests.rst](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst).
>
> Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Thank you for catching these issues. Restored shlex.quote on remote path and
drop misleading password =None.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
