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

Escape $ in getopts #683

Merged
merged 3 commits into from Mar 25, 2019

Conversation

Projects
None yet
5 participants
@maniackcrudelis
Copy link
Contributor

commented Mar 16, 2019

The problem

As found by JimboJoe here, #646 (comment), $ have to be escaped as well as double quotes.

Solution

PR Status

Ready to be review

How to test

...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@YunoHost/apps

maniackcrudelis added some commits Mar 16, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

If possible, this should be merged in the testing before an official stable release, as a fix for the current testing.

# Other ways to get that content would be to use either ${!option_var} or declare -g ${option_var}
# But... ${!option_var} can't be used as left part of an assignation.
# declare -g ${option_var} will create a local variable (despite -g !) and will not be available for the helper itself.
# So... Stop fucking arguing each time that eval is evil... Go find an other working solution if you can find one!

This comment has been minimized.

Copy link
@JimboJoe

JimboJoe Mar 16, 2019

Member

I'm in favour of a disclaimer, but maybe avoid rude words...? 😉

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Mar 16, 2019

Author Contributor

I like things to be clear...
And I'm not usually smooth and politically correct...
However, I don't care... [Politically mode ON] I'm not feeling absolutely concerned by what should be done with the text of this disclaimer and the way things should be said to be understood.

This comment has been minimized.

Copy link
@randomstuff

randomstuff Mar 16, 2019

Contributor

Still I find eval is evil :)

Show resolved Hide resolved data/helpers.d/getopts Outdated
@JimboJoe
Copy link
Member

left a comment

LGTM and tested OK 👍

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

See #685 for an alternative -- still using eval ;)

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

As discussed elsewhere, backticks should probably be escaped as well.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

\ are not escaped as well,

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

\ has been tested without problem.
Are you really sure it doesn't work !?
To be honest, you have proposed a lot of solutions so far, which were, for most of them, not working. So, I could believe that I missed something, but bring real examples of not working context.

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

\ can be passed verbatim in double quotes unless it follow some metacharacter.
In this case, the backslash quotes the metacharacters:

$ echo "\a"
\a
$ echo "\$a"
$a

Now an example of backslah:

+ ynh_debug -m '\$(ls)'
+ set +x
+ set +x
[DEBUG]ackend
debug
filesystem
getopts
ip
mysql
network
nodejs
package
print
psql
setting
string
system
user
util

What happens here is:

  • the $ is replaced by ynh_handle_getopts_args into \$
  • but the existing \ is not escaped
  • what is evaled is message="\\$(ls)"
  • the first backslash quotes the second one
  • as a result the $(ls) is not quoted.
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

???

ynh_debug -m '\$(ls)'
+ ynh_debug -m '\$(ls)'
+ set +x
+ set +x
[DEBUG] $(ls)
ynh_debug -m "\$(ls)"
+ ynh_debug -m '$(ls)'
+ set +x
+ set +x
[DEBUG] github
install_ynh-dev.sh
media
mount_directory.sh
move_to_test4.sh
move_to_test5.sh
package_check
upgradeVM.sh
use_ynh-dev.sh

I'm seriously fed up with this discussion...

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@maniackcrudelis, are you on 652edc2? (The behaviour you're getting looks like the one before 23d7a000.)

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

This example triggers injection both on the fix_getopts branch and before your change:

$ ynh_debug -m '\";ls;\"'
backend  debug  filesystem  getopts  ip  mysql  network  nodejs  package  print  psql  setting  string  system  testit.sh  user  utils
bash: \ : commande introuvable
+ set +x
[DEBUG] \
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

We have many errors on the current testing because of that error.
Could we have at least one more review and merge it quickly.

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

If you don't like the other option, manually escaping backtick, backslash, double quotes and dollar should be fine (I thinks that's all the characters which needs to be escaped):

all_args[$i]="${all_args[$i]//\\/\\\\}"
all_args[$i]="${all_args[$i]//\`/\\\`}"
all_args[$i]="${all_args[$i]//\"/\\\"}"
all_args[$i]="${all_args[$i]//$/\\\$}"
@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

I think I found a way which is actually much cleaner/simpler/easier than the one I proposed:

eval ${option_var}+='"${all_args[$i]}"'

(without any escape lines)

See the updated PR #685.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Some tests later, it doesn't looks like it breaks anything (this time...)
But as this fucking PR WORKS AS IT IS.
Let's merge it and not break anything more !

Then, as your code doesn't seem anymore to break anything, I could almost think you have tested it this time !
I think it could be merged in the next unstable, after the next stable release.
So it could be tested in all apps and be sure it really works.

@kay0u

kay0u approved these changes Mar 25, 2019

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Some tests later, it doesn't looks like it breaks anything (this time...) […] Then, as your code doesn't seem anymore to break anything, I could almost think you have tested it this time !

Well I tested the original version of the PR and it was working on all the tests I did. It's absolutely unsurprising if you know better than me how to properly test this: if you found something broken it would be a lot more constructive to actually point out what was broken.

This is actually the first time you claim something was broken in the previous version of my PR: until then you only said you were afraid something might break.

Your proposal of merging your quick fix first and take some time to test if the complete fix introduces any regression looks OK to me.

However, I don't think that ad748a75f62490386c7ca97b92191d3ee2d3fa0d is any useful and I'd say it should be either removed or reworded in order to remove all the angriness.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Reading quickly all discussions so far, I explain myself sometime, some others I didn't.
Facts are, I spent a lot of time trying your "fixes" which wasn't fixing anything or even working.
Except the first version of #685, because I have real work to do instead of wasting my time trying pseudo fix.

You want to change the behavior of this line, you have to prove it worth it, and above all, you have to be sure that works.
I've wasted a lot of time with this PR, for a fucking simple "$", which was already fixed at the beginning, as soon as JimboJoe found it.

So yes, I'm angry, because I hate wasting my time for nothing. Also, bad luck, I'm the quick to anger guy in this community. I'm not politically correct, I don't want to be and I don't care.

@maniackcrudelis maniackcrudelis merged commit ebaa16d into stretch-unstable Mar 25, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@maniackcrudelis maniackcrudelis deleted the fix_getopts branch Mar 25, 2019

@alexAubin alexAubin added this to the 3.5.x milestone Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.