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:
- install Tor Browser to /home/user/.tb/tor-browser (but any folder should work if you adjust the paths below)
- Go to https://check.torproject.org
- Save that website under /home/user/Downloads/
- 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)
Change History (51)
comment:1 Changed 4 years ago by
comment:2 follow-ups: 3 11 Changed 4 years ago by
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 follow-up: 5 Changed 4 years ago by
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
Keywords: | tbb-5.5 added |
---|
comment:5 follow-up: 6 Changed 4 years ago by
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 Changed 4 years ago by
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
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 follow-up: 10 Changed 4 years ago by
Status: | new → needs_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.
comment:9 Changed 4 years ago by
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 follow-up: 12 Changed 4 years ago by
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
comment:11 Changed 4 years ago by
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 Changed 4 years ago by
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
Keywords: | TorBrowserTeam201602R added; TorBrowserTeam201601R removed |
---|
Carrying over review tickets.
comment:14 Changed 4 years ago by
Keywords: | TorBrowserTeam201603R added; TorBrowserTeam201602R removed |
---|
comment:15 Changed 4 years ago by
Keywords: | TorBrowserTeam201604R added; TorBrowserTeam201603R removed |
---|
No easy way to do reviews in March anymore.
comment:16 Changed 4 years ago by
Keywords: | TorBrowserTeam201605R added; TorBrowserTeam201604R removed |
---|
Moving reviews over to May 2016
comment:18 Changed 4 years ago by
Keywords: | TorBrowserTeam201606R added; TorBrowserTeam201605R removed |
---|
comment:19 Changed 4 years ago by
Keywords: | TorBrowserTeam201606 removed |
---|
comment:20 Changed 3 years ago by
Keywords: | TorBrowserTeam201607R added; TorBrowserTeam201606R removed |
---|
comment:21 Changed 3 years ago by
Keywords: | TorBrowserTeam201608R added; TorBrowserTeam201607R removed |
---|
Old reviews for the "new" month.
comment:22 Changed 3 years ago by
Keywords: | TorBrowserTeam201609R added; TorBrowserTeam201608R removed |
---|
Moving review tickets to September
comment:23 Changed 3 years ago by
Keywords: | TorBrowserTeam201610R added; TorBrowserTeam201609R removed |
---|
Moving review tickets to October.
comment:24 Changed 3 years ago by
Keywords: | TorBrowserTeam201611R added; TorBrowserTeam201610R removed |
---|
Moving review tickets to November.
comment:25 Changed 3 years ago by
Keywords: | TorBrowserTeam201612R added; TorBrowserTeam201611R removed |
---|
Moving our review tickets to December.
comment:26 Changed 3 years ago by
Keywords: | TorBrowserTeam201701R added; TorBrowserTeam201612R removed |
---|
Moving review tickets to January 2017
comment:27 Changed 3 years ago by
Keywords: | TorBrowserTeam201702R added; TorBrowserTeam201701R removed |
---|
Moving review tickets.
comment:28 Changed 3 years ago by
Keywords: | TorBrowserTeam201703R added; TorBrowserTeam201702R removed |
---|
No February anymore.
comment:29 Changed 3 years ago by
Keywords: | TorBrowserTeam201704R added; TorBrowserTeam201703R removed |
---|
Moving review tickets to April.
comment:30 Changed 3 years ago by
Keywords: | TorBrowserTeam201705R added; TorBrowserTeam201704R removed |
---|
Moving review tickets to May.
comment:31 Changed 3 years ago by
Keywords: | TorBrowserTeam201706R added; TorBrowserTeam201705R removed |
---|
Moving review tickets to 201706
comment:33 Changed 2 years ago by
Keywords: | TorBrowserTeam201707R added; TorBrowserTeam201706R removed |
---|
Moving the review tickets to July 2017 as well.
comment:34 Changed 2 years ago by
Keywords: | TorBrowserTeam201707 removed |
---|
comment:35 Changed 2 years ago by
Keywords: | TorBrowserTeam201708R added; TorBrowserTeam201707R removed |
---|
Moving review tickets to August.
comment:36 Changed 2 years ago by
Keywords: | TorBrowserTeam201709R added; TorBrowserTeam201708R removed |
---|
Moving reviews to September.
comment:37 Changed 2 years ago by
Keywords: | TorBrowserTeam201710R added; TorBrowserTeam201709R removed |
---|
Moving reviews to October.
comment:38 Changed 2 years ago by
Keywords: | TorBrowserTeam201711R added; TorBrowserTeam201710R removed |
---|
Moving review to November
comment:39 Changed 2 years ago by
Keywords: | TorBrowserTeam201712R added; TorBrowserTeam201711R removed |
---|
Moving review tickets over to December
comment:41 Changed 2 years ago by
Keywords: | TorBrowserTeam201801R added; TorBrowserTeam201712R removed |
---|
Moving review tickets for real.
comment:42 Changed 23 months ago by
Keywords: | TorBrowserTeam201802R added; TorBrowserTeam201801R removed |
---|
Moving reviews to February.
comment:43 Changed 21 months ago by
Keywords: | TorBrowserTeam201803R added; TorBrowserTeam201802R removed |
---|
Moving our reviews to March 2018
comment:44 Changed 20 months ago by
Keywords: | TorBrowserTeam201804R added; TorBrowserTeam201803R removed |
---|
Moving reviews to April 2018
comment:45 Changed 20 months ago by
Keywords: | TorBrowserTeam201805R added; TorBrowserTeam201804R removed |
---|
Moving review tickets to May.
comment:46 Changed 19 months ago by
Keywords: | TorBrowserTeam201806R added; TorBrowserTeam201805R removed |
---|
Moving review tickets to June.
comment:47 Changed 18 months ago by
Component: | Applications/Tor bundles/installation → Applications/Tor Browser |
---|
Moving to Tor Browser.
comment:48 Changed 17 months ago by
Keywords: | TorBrowserTeam201807R added; TorBrowserTeam201806R removed |
---|
Moving reviews to July.
comment:49 Changed 17 months ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
Replying to adrelanos:
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.