Bug 78408 - webkit-patch upload "pretty diff" should work in cygwin
Summary: webkit-patch upload "pretty diff" should work in cygwin
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on: 40136
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-10 19:54 PST by noel gordon
Modified: 2013-04-12 00:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.83 KB, patch)
2012-02-10 20:01 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2012-02-11 15:42 PST, noel gordon
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2012-02-10 19:54:12 PST
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
Comment 1 noel gordon 2012-02-10 20:01:50 PST
Created attachment 126618 [details]
Patch
Comment 2 Adam Barth 2012-02-10 20:18:21 PST
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.
Comment 3 noel gordon 2012-02-10 20:21:29 PST
But I'm not opening a url, I'm running an win32 open command.
Comment 4 Adam Barth 2012-02-10 20:30:25 PST
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.
Comment 5 noel gordon 2012-02-11 15:40:43 PST
(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.
Comment 6 noel gordon 2012-02-11 15:42:45 PST
Created attachment 126654 [details]
Patch
Comment 7 Adam Barth 2012-02-11 16:39:51 PST
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.
Comment 8 Adam Barth 2012-02-11 16:59:22 PST
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
Comment 9 noel gordon 2013-04-12 00:40:00 PDT
(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.
Comment 10 noel gordon 2013-04-12 00:45:39 PDT
For the record, per http://docs.python.org/2/library/webbrowser.html, set environment variable BROWSER to whatever browser you want.