Refs #35530 - Dont use shellescape on the filename

[sayan3296]

Don't use shellescape on the filename for the save_to_file macro, when the filename contains a dollar ($) , Otherwise, It breaks the functionality of kickstart_networking_setup snippet.

Please refer to the issue https://projects.theforeman.org/issues/35792 for the detailed explanation

UPDATE:

Based on the detailed discussion later with the reviewers, The following has been finalized:

• By default, The filename will not be escaped ( even if it has any whitespace or special characters ) when save_to_file macro is used.
• End-users will need to process such filenames via shell_escape if any special characters need to be escaped.
• The same updates along with an example should be updated in the macro description as well.
• The test cases need to be updated to validate the first point.

[theforeman-bot]

Issues: #35792

[ekohl]

This should be Refs #35530 instead of a new issue. It's actually dealing with a regression introduced in https://projects.theforeman.org/issues/35530.

It's also a good practice to point to the commit that introduced the regression (767fea2 in this case).

[sayan3296]

This should be Refs #35530 instead of a new issue. It's actually dealing with a regression introduced in https://projects.theforeman.org/issues/35530.

It's also a good practice to point to the commit that introduced the regression (767fea2 in this case).

Hello @ekohl

Thank you so much for reviewing the PR.

I am a newbie and still adjusting to different workflows that I should be following for the PRs. But points noted. I completely agree with the second part i.e. I should have mentioned the commit details that caused the regression.

What confuses me is the request to do Refs #35530. I was reading through the contribution guidelines and it says the following:

 If your fix extends a previous fix that has not been shipped in a release, use Refs #xxxx

In this case, the commit 767fea2 has already been shipped in foreman 3.5 ( and so as with downstream satellite 6.12 GA ).

Would that not suggest using Fixes for a new issue instead of Refs for the older one?

Once I have the clarification, I can proceed further.

[ekohl]

In this case, the commit 767fea2 has already been shipped in foreman 3.5.

I completely understand that this is confusing and normally you would be right. However, 3.5.0 is only in the release candidate phase and that's a slight exception. This is actually the perfect example of where early testing of release candidates is paying off.

and so as with downstream satellite 6.12 GA

3.5 is the base for Satellite 6.13. 6.12 is based on 3.3.

[sayan3296]

In this case, the commit 767fea2 has already been shipped in foreman 3.5.

I completely understand that this is confusing and normally you would be right. However, 3.5.0 is only in the release candidate phase and that's a slight exception. This is actually the perfect example of where early testing of release candidates is paying off.

and so as with downstream satellite 6.12 GA

3.5 is the base for Satellite 6.13. 6.12 is based on 3.3.

Thanks for the clarification. I was getting mostly confused because of how the foreman branches are versioned v\s the installed version of foreman I could see on satellite 6.12.

I did a force-push after fixing the commit message and it seems to have amended it properly. I also see the PR now referenced in https://projects.theforeman.org/issues/35530 as well.

But do I need to modify the #9523 PR title manually and make the same change as the commit message?

[ekohl]

But do I need to modify the #9523 PR title manually and make the same change as the commit message?

Yes, that's manual. Our automation doesn't look at PR titles, they're just for humans but I like the consistency.

[sayan3296]

But do I need to modify the #9523 PR title manually and make the same change as the commit message?

Yes, that's manual. Our automation doesn't look at PR titles, they're just for humans but I like the consistency.

Thanks.. I am making the change soon.

It seems this test fails because I removed the escaping. ( of course, expected as it needed to escape the spaces ).

( Thinking about, under which circumstances exactly we would be needing to escape spaces from filenames )

[ekohl]

@sayan3296 I think it should be changed to a test with a $ in it to ensure that continues to work.

cc @adamruzicka

[sayan3296]

@sayan3296 I think it should be changed to a test with a $ in it to ensure that continues to work.

cc @adamruzicka

Modified the test case for $ and all checks passed now. I will wait for further instructions if there will be any.

[sayan3296]

I realized I could do better. We don't exactly need to remove the shellscape but ignore doing so when $ is in the filename

So, I went for

diff --git a/app/services/foreman/renderer/scope/macros/base.rb b/app/services/foreman/renderer/scope/macros/base.rb
index 9b7304a..8803981 100644
--- a/app/services/foreman/renderer/scope/macros/base.rb
+++ b/app/services/foreman/renderer/scope/macros/base.rb
@@ -113,7 +113,9 @@ module Foreman
             example "save_to_file('/etc/motd', \"hello\\nworld\\n\") # => 'cat << EOF-0e4f089a > /etc/motd\\nhello\\nworld\\nEOF-0e4f089a'"
           end
           def save_to_file(filename, content, verbatim: false)
-            filename = filename.shellescape
+            if filename.exclude?('$')
+              filename = filename.shellescape
+            end
             delimiter = 'EOF-' + Digest::SHA512.hexdigest(filename)[0..7]
             if content.empty?
               "cp /dev/null #{filename}"

And now, the old test case ( to escape spaces ) should work. And I have added another test case to check if there is a $ in the filename, it should not be escaped.

[adamruzicka]

Should the description reflect the different behaviour when the filename contains spaces or a dollar?

[sayan3296]

Should the description reflect the different behavior when the filename contains spaces or a dollar?

Yes.. I will update it.. as well as the commit message I believe .. Give me a sec ..

[evgeni]

I must admit I think the fix in here is wrong, as now I can't create a file called /tmp/ifcfg $sanitized_real funny because the $ will trigger the avoidance of shellescape but then the spaces will break stuff.

If we do want that the shell does post-processing of the filename, the shell calls should use "${filename}" and we should ensure that filename either does never contain double quotes or we escape those (and only those!).

[sayan3296]

I must admit I think the fix in here is wrong, as now I can't create a file called /tmp/ifcfg $sanitized_real funny because the $ will trigger the avoidance of shellescape but then the spaces will break stuff.

If we do want that the shell does the post-processing of the filename, the shell calls should use "${filename}" and we should ensure that filename either does never contain double quotes or we escape those (and only those!).

Hello @evgeni ,

Thanks for the review. And true, If you have a space and a dollar in one single filename, then That will be skipped from being shellescape'd.

But, I have two questions.

A) What are the possibilities of someone using both "space" and "dollar" in a filename that will be processed by save_to_file macro ?

( I thought about it and I could not think of any practical application of this combination hence I decided to keep it simple )

B) About your second suggestion, I tested a few variations on the shell itself. Based on the same,

We should be able to do this i.e.

# export sanitized_real="testcfg"

# echo $sanitized_real
testcfg

# cat << EOF > ifcfg-"blah ${sanitized_real}"
Testdata
EOF

# cat 'ifcfg-blah testcfg'
Testdata

Only if none of the special characters or spaces are being escaped and everything gets printed as it is without those two double quotes, Then only the OS SHELL can process them properly.

And that would mean, we need to re-think and re-work a lot on the escaping part + we will need to fix the snippet kickstart_networking_setup to use double quotes for the $sanitized_real part.

But perhaps we are making it more complicated as , I don't see save_to_file macro used anywhere else where filename contains a special character or space. It's just that snippet having /etc/sysconfig/network-scripts/ifcfg-$sanitized_real as the filename.

But i will look forward to your recommendations further.

[ekohl]

I agree we shouldn't match on input. I'd say that it should either never escape (and expect the user to call save_to_file(shell_escape(filename), content)) or have an explicit parameter. Given we have an easy shell_escape function I'd say the former is better (but make it clear in the description) than something like save_to_file(filename, content, shell_escape: true).

But perhaps we are making it more complicated as , I don't see save_to_file macro used anywhere else where filename contains a special character or space. It's just that snippet having /etc/sysconfig/network-scripts/ifcfg-$sanitized_real as the filename.

It's an API and users can have written custom templates using it.

[adamruzicka]

Maybe I have a wrong mental model about this particular macro, but for me it is an equivalent of Ruby's File.write(path, content) and the whole part about it being implemented in shell and then being interpreted by that shell feels like an implementation detail that should not be relied upon. Sadly it seems we rely on that implementation detail all over the place and can't really be rid of it.

With that being said, leaving the escaping to the user feels like the better option when compared to having an argument for it (or even matching on the input), as long as the full behaviour is documented.

[sayan3296]

To summarize what @ekohl suggested:

A) Don't hardcode filename.shellescape at all within the save_to_file macro i.e. don't escape any filenames at all by default.

B) If a user needs to escape space\any special chars in a filename, they can make use of shell_escape when creating their ruby templates i.e.

save_to_file(shell_escape('/etc/sysconfig/network-scripts/ifcfg-$sanitized_real'), ifcfg)

C) Update the PR description\Redmine\Bugzilla, to set the right expectations about points A and B.

D) Perhaps convert this test to detect that filenames are not being escaped.

[ekohl]

FYI: I want to get this in before I tag 3.5.0-rc2. So I'll probably release 3.5.0-rc2 tomorrow.

[sayan3296]

@ekohl Once you get some time, Please review if I need to make any further changes anywhere.

[ekohl]

Picked to 3.5 as 9c56caa