search

appany / helm-oci-chart-releaser

Push Helm Charts to OCI-based registries
MIT License
59 stars 13 forks source link

fix: escape args #15

Open Karitham opened 3 months ago

Karitham commented 3 months ago

fixes #14

This PR fixes the OCI action by properly escaping all the bash scripts arguments.

The reason for all of these changes is that by default, the popular container registry https://goharbor.io/ produces usernames with $ in them. Similarly, it may happen to generate passwords that have bash expensions in them.

Escaping every little thing when using the action is painful.

I've tested the fix by replacing the action line in my (private) projects with Karitham/helm-oci-chart-releaser@f2f293f8f796568f47dfaea603ae61e2b3b3fa41 and it seems to work fine when I finally remove the escaping.

If you used to escape, here are some examples after that fix:

key: before # -> after
key: "'a'" # -> "a"
key: '"a"' # -> 'a'
key: '\$a' # -> '$a' '\$a', either should work

With this PR, arguments follow the rules described here: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#2.2:~:text=a%20token%20separator.-,2.2.2%20Single%2DQuotes,-Enclosing%20characters%20in

feynmanliang commented 3 months ago

Thank you! Is this backwards compatible with already escaped arguments?

Karitham commented 3 months ago

Unfortunately if you manually escaped in your own code, no. The default state should be that you can provide strings and they just work. Escaping your passwords as a user is a countermeasure to the action being buggy, not a default state.

Because of how bash works, the same escaping rules still apply, my fix is not perfect. I'm only wrapping the args in single-quotes on the command to avoid interpolation, it's still escapable if you have a ' in your password, but that's something yaml forces you to escape on your own. I would like to address this in a follow up PR or if an issue arises.

I could implement escaping single-quotes as well, but it significantly ups the complexity. Also single quotes are fairly rare in passwords, unlike any of #$;&[]{}() which used to break before but now don't.

feynmanliang commented 3 months ago

OK, would be good semver practice to major version bump if this is unavoidable

Karitham commented 3 months ago

This is still v0, and it's a bug fix, so I disagree