Opened 4 years ago

Closed 17 months ago

#18022 closed defect (fixed)

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

Reported by: adrelanos Owned by: erinn
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201807R
Cc: whonix-devel@…, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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?

Child Tickets

Attachments (2)

execdesktop (932 bytes) - added by cypherpunks 4 years ago.
Rewritten execdesktop.
execdesktop.2 (998 bytes) - added by cypherpunks 4 years ago.
tweaked

Download all attachments as: .zip

Change History (51)

comment:1 in reply to:  description Changed 4 years ago by 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.

comment:2 Changed 4 years ago by 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?

comment:3 in reply to:  2 ; Changed 4 years ago by 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.

comment:4 Changed 4 years ago by gk

Keywords: tbb-5.5 added

comment:5 in reply to:  3 ; Changed 4 years ago by 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.

comment:6 in reply to:  5 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by 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).

comment:8 Changed 4 years ago by cypherpunks

Status: newneeds_review

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.
Last edited 4 years ago by cypherpunks (previous) (diff)

Changed 4 years ago by cypherpunks

Attachment: execdesktop added

Rewritten execdesktop.

comment:9 Changed 4 years ago by 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.

comment:10 in reply to:  8 ; Changed 4 years ago by 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

Changed 4 years ago by cypherpunks

Attachment: execdesktop.2 added

tweaked

comment:11 in reply to:  2 Changed 4 years ago by gk

Keywords: TorBrowserTeam201601R added; tbb-5.5 removed

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).

comment:12 in reply to:  10 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by gk

Keywords: TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:14 Changed 4 years ago by gk

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201602R removed

comment:15 Changed 4 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:16 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201604R removed

Moving reviews over to May 2016

comment:17 Changed 4 years ago by gk

Keywords: TorBrowserTeam201606 added

Moving reviews to June

comment:18 Changed 4 years ago by gk

Keywords: TorBrowserTeam201606R added; TorBrowserTeam201605R removed

comment:19 Changed 4 years ago by gk

Keywords: TorBrowserTeam201606 removed

comment:20 Changed 3 years ago by gk

Keywords: TorBrowserTeam201607R added; TorBrowserTeam201606R removed

comment:21 Changed 3 years ago by gk

Keywords: TorBrowserTeam201608R added; TorBrowserTeam201607R removed

Old reviews for the "new" month.

comment:22 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201608R removed

Moving review tickets to September

comment:23 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201609R removed

Moving review tickets to October.

comment:24 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611R added; TorBrowserTeam201610R removed

Moving review tickets to November.

comment:25 Changed 3 years ago by gk

Keywords: TorBrowserTeam201612R added; TorBrowserTeam201611R removed

Moving our review tickets to December.

comment:26 Changed 3 years ago by gk

Keywords: TorBrowserTeam201701R added; TorBrowserTeam201612R removed

Moving review tickets to January 2017

comment:27 Changed 3 years ago by gk

Keywords: TorBrowserTeam201702R added; TorBrowserTeam201701R removed

Moving review tickets.

comment:28 Changed 3 years ago by gk

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201702R removed

No February anymore.

comment:29 Changed 3 years ago by gk

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201703R removed

Moving review tickets to April.

comment:30 Changed 3 years ago by gk

Keywords: TorBrowserTeam201705R added; TorBrowserTeam201704R removed

Moving review tickets to May.

comment:31 Changed 3 years ago by gk

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201705R removed

Moving review tickets to 201706

comment:32 Changed 2 years ago by gk

Keywords: TorBrowserTeam201707 added

Moving Tickets to July 2017.

comment:33 Changed 2 years ago by gk

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201706R removed

Moving the review tickets to July 2017 as well.

comment:34 Changed 2 years ago by gk

Keywords: TorBrowserTeam201707 removed

comment:35 Changed 2 years ago by gk

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201707R removed

Moving review tickets to August.

comment:36 Changed 2 years ago by gk

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201708R removed

Moving reviews to September.

comment:37 Changed 2 years ago by gk

Keywords: TorBrowserTeam201710R added; TorBrowserTeam201709R removed

Moving reviews to October.

comment:38 Changed 2 years ago by gk

Keywords: TorBrowserTeam201711R added; TorBrowserTeam201710R removed

Moving review to November

comment:39 Changed 2 years ago by gk

Keywords: TorBrowserTeam201712R added; TorBrowserTeam201711R removed

Moving review tickets over to December

comment:40 Changed 2 years ago by gk

Moving review tickets to 2018

comment:41 Changed 2 years ago by gk

Keywords: TorBrowserTeam201801R added; TorBrowserTeam201712R removed

Moving review tickets for real.

comment:42 Changed 23 months ago by gk

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201801R removed

Moving reviews to February.

comment:43 Changed 21 months ago by gk

Keywords: TorBrowserTeam201803R added; TorBrowserTeam201802R removed

Moving our reviews to March 2018

comment:44 Changed 20 months ago by gk

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201803R removed

Moving reviews to April 2018

comment:45 Changed 20 months ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804R removed

Moving review tickets to May.

comment:46 Changed 19 months ago by gk

Keywords: TorBrowserTeam201806R added; TorBrowserTeam201805R removed

Moving review tickets to June.

comment:47 Changed 18 months ago by gk

Component: Applications/Tor bundles/installationApplications/Tor Browser

Moving to Tor Browser.

comment:48 Changed 17 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201806R removed

Moving reviews to July.

comment:49 Changed 17 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

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.

Note: See TracTickets for help on using tickets.