Skip to content
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

Avoid escaping paths when editing credentials #44910

Merged
merged 1 commit into from May 9, 2022

Conversation

jonathanhefner
Copy link
Member

Shellwords.escape escapes unquoted spaces with a backslash, but Windows does not treat backslash as an escape character. Escaping is also a problem when paths are expressed in shortened 8.3 format (e.g. C:\Users\RubyOn~1\AppData\Local\Temp\...) because a backslash will be erroneously added before the ~.

We can avoid the need to escape by using system(command_name, *args) instead of system(command_line), but we must still support ENV["EDITOR"] values that embed command line arguments, such as subl -w.

This commit changes to system(command_name, *args), but uses Shellwords.split to extract any embedded arguments from ENV["EDITOR"]. This requires that Windows users put quotes around the entire path of their editor if it contains spaces, such as:

SET EDITOR="C:\Program Files\Microsoft VS Code\Code.exe" -w

In other words, the following are not supported on Windows:

SET "EDITOR=C:\Program Files\Microsoft VS Code\Code.exe"
SET EDITOR=C:\Program Files\Microsoft VS Code\Code.exe
SET EDITOR=C:\"Program Files"\"Microsoft VS Code"\Code.exe -w
SET EDITOR=C:\Program^ Files\Microsoft^ VS^ Code\Code.exe -w
SET EDITOR=C:\Program` Files\Microsoft` VS` Code\Code.exe -w

Fixes #41617 (again).
Closes #44890.


This is based on my suggestion in #42728 (comment), with a fix for the test I mentioned in #42728 (comment).

/cc @ariccio @joshleblanc Would it be possible for you to test this change locally, and verify that it solves your issue?

`Shellwords.escape` escapes unquoted spaces with a backslash, but
Windows does not treat backslash as an escape character.  Escaping is
also a problem when paths are expressed in shortened 8.3 format (e.g.
`C:\Users\RubyOn~1\AppData\Local\Temp\...`) because a backslash will be
erroneously added before the `~`.

We can avoid the need to escape by using `system(command_name, *args)`
instead of `system(command_line)`, but we must still support
`ENV["EDITOR"]` values that embed command line arguments, such as
`subl -w`.

This commit changes to `system(command_name, *args)`, but uses
`Shellwords.split` to extract any embedded arguments from
`ENV["EDITOR"]`.  This requires that Windows users put quotes around the
entire path of their editor if it contains spaces, such as:

```
SET EDITOR="C:\Program Files\Microsoft VS Code\Code.exe" -w
```

In other words, the following are **not** supported on Windows:

```
SET "EDITOR=C:\Program Files\Microsoft VS Code\Code.exe"
SET EDITOR=C:\Program Files\Microsoft VS Code\Code.exe
SET EDITOR=C:\"Program Files"\"Microsoft VS Code"\Code.exe -w
SET EDITOR=C:\Program^ Files\Microsoft^ VS^ Code\Code.exe -w
SET EDITOR=C:\Program` Files\Microsoft` VS` Code\Code.exe -w
```

Fixes rails#41617 (again).
Closes rails#44890.
@rails-bot rails-bot bot added the railties label Apr 17, 2022
@joshleblanc
Copy link
Contributor

This change works on my end, and is a better solution than mine.

FWIW, your assumption that the temp path was coming from the environment variables was correct. It's set with the 8.3 filename in both TMP and TMP.

@byroot byroot merged commit 38d5846 into rails:main May 9, 2022
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Jul 24, 2022
In rails#44910, the `credentials:edit` command was fixed to support spaces in
the temporary file path on Windows.  This commit applies the same fix to
the `encrypted:edit` command.
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Jul 30, 2022
In rails#44910, the `credentials:edit` command was fixed to support spaces in
the temporary file path on Windows.  This commit applies the same fix to
the `encrypted:edit` command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change_credentials_in_system_editor breaks with spaces in Windows user name
3 participants