1176033 - When mozregression provides a "resume" command, it needs to quote and/or escape special characters in the user-provided -a/--arg parameter, to handle URLs with "&" and other special characters

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Run mozregression with an "-a" arg, providing a URL that has a & in it. e.g.:

mozregression -a  "https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ceba6484abda" --good=2015-05-01 --bad=2015-06-15

Note that the quotes around the URL are very important here. They prevent my shell from interpreting the "&" as meaning "run in the background, and then run this command".

 (2) Interrupt it while it's downloading a build.
 (3) Inspect its "To resume" instructions

ACTUAL RESULTS:
 0:03.19 LOG: Dummy-14 Bisector INFO To resume, run:
 0:03.19 LOG: Dummy-14 Bisector INFO mozregression --good=2015-05-01 --bad=2015-06-15 --app=firefox --arg=https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ceba6484abda --bits=64

Note that it's missing quotes around the URL.

EXPECTED RESULTS:
Quotes around the "--arg" value.


NOTE:
If I actually run this command, I get the following output:
[dholbert@indigo:~]$ mozregression --good=2015-05-01 --bad=2015-06-15 --app=firefox --arg=https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ceba6484abda --bits=64
[1] 7987
--bits=64: command not found
[dholbert@indigo:~]$  0:01.42 LOG: MainThread Bisector INFO Downloading build from: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/05/2015-05-24-03-02-34-mozilla-central/firefox-41.0a1.en-US.linux-x86_64.tar.bz2
===== Downloaded 100% =====


"[1] 7987" is my shell telling me that it's backgrounded a process (due to the "&".

"--bits=64" is my shell telling me it *tried to run '--bits=64' as its own separate command". (!!)

Comment 1

[Daniel Holbert [:dholbert]]

(Additionally, any quotes that happen to be inside of the URL probably need to be escaped, I think.)

Comment 2

[Daniel Holbert [:dholbert]]

More contrived, but also worth worrying about: dollar signs in the URL. Those have a special meaning unless they're escaped (maybe even inside quotes?)  And other symbols like "`" and "#", too.

EXAMPLE WITH DOLLAR SIGNS: if you run this command:
 mozregression --good=2015-05-01 -a=https://mozilla.org/\$USER
it pops up Firefox instances, loading the URL "https://www.mozilla.org/en-US/$USER" as you'd expect.

But if you interrupt that command, the suggested resume command is:
mozregression --good=2015-05-01 --bad=2015-06-18 --app=firefox --arg=https://mozilla.org/$USER --bits=64

And *that* command will pop up Firefox instances that load https://www.mozilla.org/en-US/dholbert (or whatever your username is).  Depending on the particular $FOO environmental variable, this may end up leaking private data or do something very unexpected.

Comment 3

[Daniel Holbert [:dholbert]]

I have mozregression version 0.36 (according to mozregression --version), which is the latest that pip will give me.

Comment 4

[Julien Pagès (:parkouss)]

Thanks Daniel for the bug report! It is clearly missing some url escaping here.

We could fix it properly. Also, I remember I opened a bug some time ago (bug 1138145) because it is not the first time the resume info is broken;

So I would like your opinion here: does it matter to you if we only print the --good/--bad or --good-rev/--bad-rev options, or do you think it is better to print the whole options the user entered ?

I'm in favor of just saying something like:

Resume info: run your last command using --good=2015-06-20 --bad 2015-06-23
(instead of all options)

To me, it is simple enough. But if there is any interest to somebody, we can do otherwise.

What do you think ?

Comment 5

[Daniel Holbert [:dholbert]]

Sure, I'd be happy having it just omit the "--arg" option (since it's hard to get the escaping right in all edge-cases).

I think you may need more than just --good/--bad in general, e.g. if I started bisecting Nightlies and then mozregression gets interrupted while it's progressed to bisecting inbound builds. (at which point it needs some more nuanced args to resume properly I think)

But yeah, that basically sounds good.

Comment 6

[Julien Pagès (:parkouss)]

Yeah, I was thinking --good/--bad, and in case we started inbound (or switched to inbound) then print --good-rev/--bad-rev.

Will, a preference here ?

Comment 7

[William Lachance (:wlach)]

Ideally we would print the entire thing so the user can cut & paste easily, but using a subset is better than printing a command that will fail. I don't think fixing the printout to escape stuff should be that hard?

Comment 8

[Julien Pagès (:parkouss)]

agreed - let fix this properly. :)

Comment 9

[Julien Pagès (:parkouss)]

Attachment: fix resume info escaping

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8624520 [details] [review]
fix resume info escaping

LGTM!

Comment 11

[Julien Pagès (:parkouss)]

Thanks Will!

I merged this in: https://github.com/mozilla/mozregression/commit/ac551d09b47c89c2efb917f75cd062e9ea972ee2

I think we could do a release soon - I'll say next week. Also, a gui release would be great, there are some great things added recently!