[webkit-reviews] review denied: [Bug 78408] webkit-patch upload "pretty diff" should work in cygwin : [Attachment 126654] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied noel gordon <noel.gordon at gmail.com>'s
request for review:
Bug 78408: webkit-patch upload "pretty diff" should work in cygwin
https://bugs.webkit.org/show_bug.cgi?id=78408

Attachment 126654: Patch
https://bugs.webkit.org/attachment.cgi?id=126654&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.