Special characters in password breaks repository initialization

[DistortedBit]

V 6.2

Bug: Create new repo with password

as'

Command fails.

It seems here's no escaping going on here

https://github.com/openmediavault/openmediavault/blob/master/deb/openmediavault/usr/share/php/openmediavault/system/process.inc#L164

Which is used to execute the built command.

Try it again with the following password again. It works.

as1

[DistortedBit]

Upon further investigation, I found this is the specific commit that broke the password escaping

https://github.com/OpenMediaVault-Plugin-Developers/openmediavault-borgbackup/commit/c13b6ba9fd0d463e6773da2380fa014c87bc0177

Passwords work in 6.1.3. After I updated, now I can't add the same repository anymore.

[ryecoaaron]

Yep, I knew where I broke. I was thinking the setEnv function escaped the variable value but it doesn't. Can you try the updated version in the testing folder?

rm -f openmediavault-borgbackup_6.2_all.deb wget https://omv-extras.org/testing/openmediavault-borgbackup_6.2_all.deb sudo dpkg -i openmediavault-borgbackup_6.2_all.deb

[DistortedBit]

Yeah, you'd think so as it's a part of the core library. I actually went to omv repo and was about to open an issue, but it said unless a moderator in the forum asked to, issues shouldn't be logged there.

Thank you for the quick fix. I'll be trying this out shortly. I don't see any new commit in the repo or is it just local?

[ryecoaaron]

I don't know that it is actually a bug in the core library. I had never noticed setEnv existed until I added it in the last change. Since I can escape what I am passing, I think that is fine.

I didn't push the change until I had confirmation it was working. Would you prefer that I push it so you can see it?

[DistortedBit]

I just tested it. It's working well. Well done.

Imo, we can still use the core lib. We just need to subclass it and override getCommand method here

https://github.com/openmediavault/openmediavault/blob/master/deb/openmediavault/usr/share/php/openmediavault/system/process.inc

And take care of the env escaping ourselves. When it's fixed upstream, we can just drop the subclass.

Pushing the code as a separate branch will help me review the code so I will be able to give you more insights before we test it out. If it's too much trouble then it's fine.

Thank you again for the fix. Any time frame when this'll hit stable?

[ryecoaaron]

I am still using setEnv. I just escapeshellarg the passphrase before passing it. I pushed the commit.

I will push it to the repo in a little bit.

[DistortedBit]

I am still using setEnv. I just escapeshellarg the passphrase before passing it. I pushed the commit.

I will push it to the repo in a little bit.

Imo this is a BAD idea. Because when this gets fixed in upstream, you will have to track down EVERY setEnv call and edit it. It'll certainly cause issues in different ends.

I'd say if you really don't want to override the getCommand method because it's too complicated, override the setEnv method and apply escaping there. When fixed upstream you'll just need to edit one method in one file.

As an added benefit for future development it'll also take away the overhead of escaping every input manually until this is fixed upstream.

This is just general good practice advice, may or may not be appropriate here. I'd send you a pull request but I just don't have the time to setup the development environment right now.

[ryecoaaron]

Too complicated? nope. Just didn't want to spend the time. escapeshellarg works and is all over many of my plugins. And if setEnv was escaped, then many things with command would need to be escaped. I would have to change a lot of plugins.

[DistortedBit]

Fair enough. I see it's live now. Thank you for the fix.

[ryecoaaron]

@DistortedBit 6.2 is in the repo now. And if you feel like contributing to any plugin, feel free.