start-tor-browser.desktop parameter passing broken on spaces

[adrelanos]

Steps to reproduce:

1. install Tor Browser to /home/user/.tb/tor-browser (but any folder should work if you adjust the paths below)
2. Go to ​https://check.torproject.org
3. Save that website under /home/user/Downloads/
4. There now will be a file "/home/user/Downloads/Congratulations. This browser is configured to use Tor..html" (any other long file name with spaces should work also)

The following commands to work as expected:

bash -x /home/user/.tb/tor-browser/Browser/start-tor-browser --new-tab "/home/user/Downloads/Congratulations. This browser is configured to use Tor..html"

bash -x /home/user/.tb/tor-browser/Browser/start-tor-browser "/home/user/Downloads/Congratulations. This browser is configured to use Tor..html"

firefox "/home/user/Downloads/Congratulations. This browser is configured to use Tor..html"

The following fails:

/home/user/.tb/tor-browser/start-tor-browser.desktop "/home/user/Downloads/Congratulations. This browser is configured to use Tor..html"

Expected result:
Open that file and show it in Tor Browser as the other above commands working for Firefox / Tor Browser should do.

Actual result:
Tor Browser opens ~ 10 links that do not work. To Congratulations., This, etc.

Impact:
This currently prevents Whonix users [Whonix is using Tor Browser by default] to open local html files.

Workaround:
We could use Browser/start-tor-browser directly instead, which would work, if that is now the canonical way to do this.

But perhaps parameter passing can be fixed in start-tor-browser.desktop?

[gk]

Replying to adrelanos:

But perhaps parameter passing can be fixed in start-tor-browser.desktop?

Sounds like a good idea. A patch is welcome! Bonus points if there is something to review within the next two weeks as it will make it into Tor Browser 5.5 then.

[adrelanos]

Likely the bug lies in:
​https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/RelativeLink/execdesktop

The following line seems wrong.

eval "${TBB_START_PROG} $@"

Replacing it with the following does the trick.

eval "${TBB_START_PROG}" \'$@\'

If that looks sane to you, could you please apply it to git?

[cypherpunks]

Replying to adrelanos:

Likely the bug lies in:
​https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/RelativeLink/execdesktop

The following line seems wrong.

eval "${TBB_START_PROG} $@"

Replacing it with the following does the trick.

eval "${TBB_START_PROG}" \'$@\'

If that looks sane to you, could you please apply it to git?

Wouldn't

eval "${TBB_START_PROG}" "${@}"

also work? It avoids escaping things which is ugly IMO.

[adrelanos]

Replying to cypherpunks:

Wouldn't

eval "${TBB_START_PROG}" "${@}"

also work? It avoids escaping things which is ugly IMO.

No, that does not work.

[cypherpunks]

Replying to adrelanos:

Replying to cypherpunks:

Wouldn't

eval "${TBB_START_PROG}" "${@}"

also work? It avoids escaping things which is ugly IMO.

No, that does not work.

After some testing it seems the use of eval makes my solution not work because it concatenates its arguments into one command and executes the result. The concatenation step removes the string boundaries and makes TBB see arguments with spaces as separate arguments.

Simply removing the eval does not work either.

[cypherpunks]

More testing shows that it does not work without eval because of the default '--detach' argument which got added in #15741.

Replacing the line with

${TBB_START_PROG} "${@}"

does seem to work. (Note that there is no eval and the first variable is not quoted).

[cypherpunks]

Here goes a replacement "execdesktop" for consideration.

Differences:

• Better documented, including some error checking.
• In the subshell used to get the value of "X-TorBrowser-ExecShell": a single fork+exec instead of four pipelined.
• Fixes what I think is a bug in the current implementation: the sed used to remove field codes (see dektop entry spec) is missing the global flag, so it will only replace the first one. (Or was this the intended behaviour?)
• Actually execs, so that there's no useless shell lingering.
• Tabs for indentation. Wraps at 80 columns.
• No branching on number of arguments.
• More elegant code, if I may say so myself.

[cypherpunks]

Rewritten execdesktop.

[cypherpunks]

Forgot to say: this implementation, in addition to properly handling arguments with spaces, supports spaces in the ExecShell command. So having something like
X-TorBrowser-ExecShell=./path\ with\ spaces/executable -o --longopt
works as expected.

Also, this implementation quotes its "$1" argument, so that path can also have spaces. This is a potential bug in the current implementation, since all paths should be quoted.

[cypherpunks]

Replying to cypherpunks:
howdy, fellow sedder

• More elegant code, if I may say so myself.

elegance in shell script? ;)

this is a tweaked version, slightly more .desktop-compliant

[cypherpunks]

tweaked

[gk]

Replying to adrelanos:

Likely the bug lies in:
​https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/RelativeLink/execdesktop

The following line seems wrong.

eval "${TBB_START_PROG} $@"

Replacing it with the following does the trick.

eval "${TBB_START_PROG}" \'$@\'

If that looks sane to you, could you please apply it to git?

That's not working for me if I do things like ./start-tor-browser.desktop --debug --log. So, it seems for Tor Browser 5.5 you need to use start-tor-browser directly which seems like a good idea anyway.

The execdesktop "rewrites" should get tested in the alpha series first, I think (in case one of the proposals passes review).

[cypherpunks]

Replying to cypherpunks:

Replying to cypherpunks:
howdy, fellow sedder

• More elegant code, if I may say so myself.

elegance in shell script? ;)

Well, if not elegance, at least some sanity and consistency. Look at the mess that is "start-tor-browser": was obviously hacked together by several people, with some of them clearly not understanding how to write bash script.

this is a tweaked version, slightly more .desktop-compliant

I like it.

gk: I had to change some stuff in "start-tor-browser" (the log path selection is, uhh, "cognitively challenged"). I might rewrite other parts as well if you are interested.

[gk]

Carrying over review tickets.

[gk]

No easy way to do reviews in March anymore.

[gk]

Moving reviews over to May 2016

[gk]

Moving reviews to June

[gk]

Old reviews for the "new" month.

[gk]

Moving review tickets to September

[gk]

Moving review tickets to October.

[gk]

Moving review tickets to November.

[gk]

Moving our review tickets to December.

[gk]

Moving review tickets to January 2017

[gk]

Moving review tickets.

[gk]

No February anymore.

[gk]

Moving review tickets to April.

[gk]

Moving review tickets to May.

[gk]

Moving review tickets to 201706

[gk]

Moving Tickets to July 2017.

[gk]

Moving the review tickets to July 2017 as well.

[gk]

Moving review tickets to August.

[gk]

Moving reviews to September.

[gk]

Moving reviews to October.

[gk]

Moving review to November

[gk]

Moving review tickets over to December

[gk]

Moving review tickets to 2018

[gk]

Moving review tickets for real.

[gk]

Moving reviews to February.

[gk]

Moving our reviews to March 2018

[gk]

Moving reviews to April 2018

[gk]

Moving review tickets to May.

[gk]

Moving review tickets to June.

[gk]

Moving to Tor Browser.

[gk]

Moving reviews to July.

[gk]

Okay, sorry for the really long delay. We go for now with the much simpler patch mentioned in comment:7. This got fixed with #26951.

I opened #26995 for the suggested execdesktop enhancements.