puppetlabs-docker icon indicating copy to clipboard operation
puppetlabs-docker copied to clipboard

docker_run_flags: Shellescape any provided values

Open neomilium opened this issue 2 years ago • 18 comments

When passing an environment variable with special characters, it can broke the script or produce unexpected behavior. I encountered two of them:

  • If a $ is in environment variable, its interpolated as a variable
  • If a " is in environment variable, it broke the end of string, so the script

This PR contains a commit to escape any value that will be pushed in the docker start script.

neomilium avatar Mar 28 '22 17:03 neomilium

docker_run_flags is a function

Breaking changes to this file WILL impact these 1 modules (exact match):
Breaking changes to this file MAY impact these 6 modules (near match):

This module is declared in 6 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 28 '22 17:03 CLAassistant

Hey 👋

Can you set this PR to ready for review?

thanks!

chelnak avatar Mar 28 '22 21:03 chelnak

I am wondering if the CI failures are relevant… Like volume-1:C:\volume_1 being escaped as volume-1:C:\\volume_1… Not sure about how to check this…

smortex avatar Mar 29 '22 03:03 smortex

@chelnak Test added by @smortex, PR is now ready.

neomilium avatar Mar 31 '22 09:03 neomilium

@smortex - i'll take a closer look at this tomorrow. Seems odd that all of the failures here are for Windows and they don't seem to be on the nightly tests either..

chelnak avatar Apr 03 '22 19:04 chelnak

@neomilium @chelnak I added a commit that seems to help… but escaping on windows is total nonsense and I am sure of nothing :scream:

Some windows node pass, the one failing seems to fail earlier than the code added / changed here and the failures might be the result of some external service not being reliable… maybe?

smortex avatar Apr 13 '22 02:04 smortex

Last CI failures seems to be caused by inaccessible windows test nodes. I think this is ready for review.

smortex avatar Jun 01 '22 01:06 smortex

Failed jobs have been restarted. If everything looks ok I’d like to get this in to the next release.

chelnak avatar Jun 01 '22 07:06 chelnak

@neomilium have you had a chance to see if the issue you originally tried to fix with this PR is indeed fixed with all these changes?

smortex avatar Jun 01 '22 07:06 smortex

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

canihavethisone avatar Jun 11 '22 03:06 canihavethisone

@smortex @chelnak @bastelfreak - why has stdlib been raised to min v8.2.0 in the pull request? That is very high and will have breaking impact when combined with a number of other available modules

Because we had to add escape functions for windows in the stdlib: see this discussion above and https://github.com/puppetlabs/puppetlabs-stdlib/pull/1235.

smortex avatar Jun 11 '22 04:06 smortex

@smortex @bastelfreak How do you both feel about moving forward with this PR?

Pending test failures which I think may be transient, I think it is good.

Though I do wonder if it is a breaking change due to the higher requirements of stdlib.

What are your thoughts?

chelnak avatar Jul 04 '22 10:07 chelnak

@smortex

I've chatted with @canihavethisone around some of the concerns raised in the comment above.

I personally think stdlib is the right place for the new escape commands however I want to get more of a feel for the potential impact on other users of the module.

This is also an issue that we will probably hit again with other modules. In order to take advantage of newer features in stdlib, the minimum version number will need to be raised.

chelnak avatar Jul 04 '22 13:07 chelnak

My concern is that the major fast forward of the stdlib dependency may have significant impact for users of this module in deployments alongside other modules that haven't caught up yet. I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped. I understand there will be diversity of opinions but think its worth considering.

canihavethisone avatar Jul 04 '22 13:07 canihavethisone

I recon a good metric would be how many popular modules still have stdlib pinned lower than v.8x, and weigh that against the urgency of this change or if it can be merged when 8x is more commonly scoped.

How to define "popular modules"?

neomilium avatar Jul 05 '22 14:07 neomilium

@chelnak @smortex Hi, im not concerned about people blindly updating, I'm concerned that we will lock out users who use this module alongside numerous others. Locking them out will deny them of new features and bugfixes until their other dependencies support stdlib v8. Bottom line IMO is that there is a huge difference between setting the upper limit to v8 or the lower requirement to v8. Its also a cost v benefit consideration at this moment in v8's youth.

canihavethisone avatar Aug 18 '22 22:08 canihavethisone

Here's a list of all the Forge modules that pin their stdlib dependencies to <8.x, sorted by download count (a weak correlation to popularity.) Notice that many of them are deprecated.

https://docs.google.com/spreadsheets/d/16QT1wd5CmwJL9TbKe4GZgpPi3W4nXHo7Q4fD5aK8bRM/edit#gid=223629225

binford2k avatar Aug 19 '22 15:08 binford2k

Hi @neomilium. After careful evaluation of your PR, I'm happy to inform you that we are ready to go ahead and merge it. Thanks for your contribution!

We also noted that there are some concerns about this PR, such as the one raised by @canihavethisone. We have considered the potential impact of these changes and we have decided to still go forward with it. While it is true that there may be some compatibility issues with a few non-supported modules, we also have noted that this change may be important for one of our current security enhancement projects and, as such, either this or something very similar was bound to be implemented sooner or later. We are sorry if this causes any inconvenience.

LukasAud avatar Oct 12 '22 09:10 LukasAud

This PR will be temporarily reverted until a release can be cut. Then it will be reinstated. This process is done to follow with standard best practices.

LukasAud avatar Oct 12 '22 10:10 LukasAud

Hey @neomilium. We are sorry for this inconvenience but we had to revert your PR in order to follow best practices when releasing a new version of our module, by making a minor release prior to a backwards incompatible one. We could re-implement it but I'm afraid it would appear under our name. Could you reopen your branch under a new PR so that we can merge and give proper credit to all contributors?

LukasAud avatar Oct 28 '22 15:10 LukasAud

Rebasing or cherry-picking the commits would maintain the original commit authors.

kenyon avatar Oct 28 '22 21:10 kenyon