ynh_replace_string and others not escaping & and \ for sed

[MayeulC]

Describe the bug

Found while upgrading synapse, the shared secret contained a "&" character.

According to the GNU sed manual on the s command:

The replacement can contain \n (n being a number from 1 to 9, inclusive) references, which refer to the portion of the match which is contained between the nth \( and its matching \). Also, the replacement can contain unescaped & characters which reference the whole matched portion of the pattern space.
[...]
To include a literal \, &, or newline in the final replacement, be sure to precede the desired \, &, or newline in the replacement with a \.

To reproduce

$ echo "test: __TO_REPLACE__" | sed 's@__TO_REPLACE__@replac&ment@g'
test: replac__TO_REPLACE__ment

Or try to use ynh_replace_string with an "&" in the replacement.

Expected behavior

& and \ should be properly escaped.

Affected code

https://github.com/YunoHost-Apps/Experimental_helpers/blob/f0c9070299313b8fc4c03dabceed110a052a0c81/ynh_add_config/ynh_add_config#L125-L139

https://github.com/YunoHost/yunohost/blob/6fc6a2ba4c17434803cea2553a3dc7c51cedd338/data/helpers.d/string#L48-L51

Suggested fixes

The first affected block should arguably use the helper from the second link.

Inspired by this answer, matches could be escaped.
Do we want to handle complex substitution? Like __SOME.OTHER.STRING_? . there will match any character.
If that's not necessary, changing the replace_string might suffice:

replace_string=$( printf '%s' "$replace_string" | sed -e 's/[@&]/\\&/g')

(@ above assumes the separator is @ for sed)

However, sed is really the worst tool for that kind of thing. Python is already a dependency, so why not use it? Alternatively, it might be worth it to submit a patch to GNU sed. tr would be a better tool, but it operates on streams, not files.

I'll see if I can submit a PR later, but feel free to fix in the meanwhile.

[tituspijean]

I'm wondering if that would be actually the job of ynh_replace_special_string?

[MayeulC]

Hmm, maybe, but if you want to use special sed syntax, you probably want to use sed directly.

Moreover, the name of ynh_replace_string doesn't make it obvious that it will honour regexes and references. The latter is more of a special case. In turn, ynh_replace_special_string should probably be the default. The name makes me think about a special case, but in turn, it's as basic as a replacement can get.

One thing is sure, though: as-is, ynh_replace_vars should use ynh_replace_special_string.

Also, as in my original point, ynh_replace_special_string should do a better job of escaping characters such as [].

[Gredin67]

@tituspijean @zamentur what would be the correct solution to this ? Something in core or something in the apps packages?

[zamentur]

Recent fixes has been made :

• to escape & in ynh_write_var_in_files. However the other issues are still there YunoHost/yunohost@9f686a1
• to be able to put @ into ynh_replace_string YunoHost/yunohost@f742bdf

But other string issues could occurred like [ or ?