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.

Sign up for GitHub

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

640 triaging issue #642

Open
wants to merge 2 commits into
base: rewrite_for_6.0
Choose a base branch
from
Open

640 triaging issue #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da5cbb9
Triageing issue #640
alessandro-fazzi Feb 12, 2022
c9ac5f2
User reported the error while pull-ing
alessandro-fazzi Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ruby.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ jobs:
- name: Install dependencies
run : bundle install
- name: Run tests
run: bundle exec rake
run: bundle exec rspec spec/actions/run_remote_command_spec.rb:95
41 changes: 40 additions & 1 deletion spec/actions/run_remote_command_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
let(:bad_command) { 'exit 1' }

before do
silence_logger!
allow(context[:photocopier])
.to receive(:exec!).with(good_command)
.and_return([nil, nil, 0])
Expand All @@ -18,6 +17,8 @@
end

it 'works like it should' do
silence_logger!

result = described_class.execute(
photocopier: context.fetch(:photocopier),
cli_options: context.fetch(:cli_options),
Expand All @@ -30,6 +31,8 @@

context 'when it fails' do
it 'sets the expected error message into result' do
silence_logger!

result = described_class.execute(
photocopier: context.fetch(:photocopier),
cli_options: context.fetch(:cli_options),
Expand All @@ -44,6 +47,8 @@

context 'when `--simulate`' do
it 'does not execute the command and result is successful' do
silence_logger!

context[:cli_options][:simulate] = true

result = described_class.execute(
Expand All @@ -56,4 +61,38 @@
expect(result).to be_success
end
end

context 'testing @castilma\'s big report' do
let(:command) do
'mysqldump --host=remote_database_host --user=user ' \
Copy link

Choose a reason for hiding this comment

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

Where is the code that creates this commandline normally? The escaping may happen when the commandline is constructed.

Copy link

Choose a reason for hiding this comment

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

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

found it:here
(the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets.
So the regex used for substituting secrets should search for shellescaped secrects!

Another potential issue:
Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input.
I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings.
That way you don't need to worry about string escaping! You just pass an array of the used arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

No problem; you'd be welcome anyway :)

found it:here (the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets. So the regex used for substituting secrets should search for shellescaped secrects!

This seems a good catch to me! But it seems more an endemic problem the fact we're logging escaped commands more than thinking about searching for escaped secrets. I have to tinker a bit on this.

Another potential issue: Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Why do you say system will call «whatever the user's current shell is»? In my knowledge system calls, in any form, should use the standard shell; which means, in *nix systems, /bin/sh. Would you mind to point me where you found that information? TIA

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input. I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings. That way you don't need to worry about string escaping! You just pass an array of the used arguments.

If your point is that it's difficult to use system due to the occurring shell expansion, keep in mind that the method has the same overloaded signature as exec. If that was not your point then I'm not catching it, sorry.

Instead I'm not prone to introduce threaded patterns (fork) because I've not enough knowledge to control them and sincerly I do not take what benefit it should bring on the table here. But I'm not interested in preventing a discussion about that if I am missing something you consider important :)

Thanks for the effort; I'll be on this as soon as I'll have a chunk of spare time

Copy link

Choose a reason for hiding this comment

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

The system() documentation refers to exec(), which uses /bin/sh by default, but only after looking the environment variable RUBYSHELL.
(So I was wrong/unclear, but am right now :))

I did not notice that one could use system() without a subshell. So, one could just change to use system() with each argument separately passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the patience and feedback.

I've never used that signature of #system, but my feeling is that it could be more functional and more straight to read/write.

I'll deepen on it a bit and will try to bring out something useful. For sure I won't target the 5.x version BTW.

Copy link

Choose a reason for hiding this comment

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

If you want to do this in sometime, don't just forget it, because the stale bot closed the issue.

'--password=R4ndom#+Str1nG ' \
'--result-file="/var/www/your_site/wp-content/dump.sql" database_name'
end

let(:context) do
OrganizerContextFactory.make_for(
described_class,
:pull,
cli_options: {
config: movefile_path_for('with_secrets_castilma'),
environment: :remote
}
)
end

it 'censors the password on STDOUT' do
allow(context[:photocopier])
.to receive(:exec!).with(command)
.and_return([nil, nil, 0])

expect do
described_class.execute(
photocopier: context.fetch(:photocopier),
cli_options: context.fetch(:cli_options),
logger: context.fetch(:logger),
command: command
)
end.to output(/--password=\[secret\]/).to_stdout_from_any_process
end
end
end
23 changes: 23 additions & 0 deletions spec/fixtures/movefiles/with_secrets_castilma
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
global:
sql_adapter: "wpcli"
local:
wordpress_path: "~/dev/sites/your_site"
remote:
vhost: "http://secrets.example.com"
wordpress_path: "/var/www/your_site"
database:
name: "database_name"
user: "user"
password: "R4ndom#+Str1nG"
host: "remote_database_host"
ssh:
user: "user"
password: "ssh_password"
host: "ssh_host"
port: 30000
ftp:
user: "user"
password: "ftp_password"
host: "ftp_host"
foo:
vhost: "https://foo.bar"