Bug 78408 – webkit-patch upload "pretty diff" should work in cygwin

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[noel gordon]

webkit-patch upload "pretty diff" should work in cygwin[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[-[C[4~[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C[C

[noel gordon]

Created attachment 126618 [details]
Patch

[Adam Barth]

Comment on attachment 126618 [details]
Patch

This logic should be in user.py so it can be shared by everyone who tries to open a URL.

[noel gordon]

But I'm not opening a url, I'm running an win32 open command.

[Adam Barth]

Comment on attachment 126618 [details]
Patch

I'm not sure this problem is worth solving.  Even if we agree that it's worth solving, this isn't the correct approach.  All the code at this layer is abstracted away from the underly operating system via the "tool" object.  Any decisions about cygwin or creating processes needs to be below the tool abstraction.

[noel gordon]

(In reply to comment #4)
> (From update of attachment 126618 [details])
> I'm not sure this problem is worth solving.

I'd like to see a diff if I happen to be in cygwin, like I do in the other webkit-patch environments I use.  So if you want something done ...

(In reply to comment #2)
> (From update of attachment 126618 [details])
> This logic should be in user.py so it can be shared by everyone who tries to open a URL.

Righto tried that, PTAL.

[noel gordon]

Created attachment 126654 [details]
Patch

[Adam Barth]

Comment on attachment 126654 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126654&action=review

> Tools/Scripts/webkitpy/common/system/user.py:158
> +        if url.startswith("file:///"):
> +            cygwin_root = commands.getoutput("cygpath -a -m /")
> +            url = url.replace("file:///", "file:///%s/" % cygwin_root, 1)
> +        open_default_viewer_command = 'cmd.exe /c "start %s"' % url
> +        status = commands.getstatusoutput(open_default_viewer_command)

I appreciate your desire to improve the tools to make them do what you want, but this implementation contains security bugs.  Throughout webkitpy, we've avoiding using string concatenation and ShellExecute to create subprocesses.  For example, if |url| contains the following string:

file:///User/noel"; rm -rf /; echo "

you'll end up deleting your entire harddrive.  To avoid these problems, we have the Executive abstraction for creating subprocesses.  Executive, in turn, is built on subprocess.Popen, which lets us avoid the shell and its attendant security risks.

[Adam Barth]

What happens if the URL points to something other than an HTML file, like a Python file or an EXE?  Isn't |start| essentially ShellExecute again?  Doesn't that mean we'll execute arbitrary code?

http://www.computerhope.com/starthlp.htm

[noel gordon]

(In reply to comment #7)
 
> I appreciate your desire to improve the tools to make them do what you want, but this implementation contains security bugs.  Throughout webkitpy, we've avoiding using string concatenation and ShellExecute to create subprocesses.  For example, if |url| contains the following string:
> 
> file:///User/noel"; rm -rf /; echo "

Try a similar |url| using using pre-existing implementation: webbrowser.open(url) might not be your friend.

[noel gordon]

For the record, per http://docs.python.org/2/library/webbrowser.html, set environment variable BROWSER to whatever browser you want.