78408 2012-02-10 19:54:12 -0800 webkit-patch upload "pretty diff" should work in cygwin 2013-04-12 00:45:39 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 40136 1 noel.gordon noel.gordon abarth ojan webkit.review.bot oldest_to_newest 554555 0 noel.gordon 2012-02-10 19:54:12 -0800 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 554559 1 126618 noel.gordon 2012-02-10 20:01:50 -0800 Created attachment 126618 Patch 554560 2 126618 abarth 2012-02-10 20:18:21 -0800 Comment on attachment 126618 Patch This logic should be in user.py so it can be shared by everyone who tries to open a URL. 554561 3 noel.gordon 2012-02-10 20:21:29 -0800 But I'm not opening a url, I'm running an win32 open command. 554564 4 126618 abarth 2012-02-10 20:30:25 -0800 Comment on attachment 126618 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. 554720 5 noel.gordon 2012-02-11 15:40:43 -0800 (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. 554722 6 126654 noel.gordon 2012-02-11 15:42:45 -0800 Created attachment 126654 Patch 554730 7 126654 abarth 2012-02-11 16:39:51 -0800 Comment on attachment 126654 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. 554733 8 abarth 2012-02-11 16:59:22 -0800 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 874411 9 noel.gordon 2013-04-12 00:40:00 -0700 (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. 874414 10 noel.gordon 2013-04-12 00:45:39 -0700 For the record, per http://docs.python.org/2/library/webbrowser.html, set environment variable BROWSER to whatever browser you want. 126618 2012-02-10 20:01:50 -0800 2012-02-11 15:42:36 -0800 Patch bug-78408-20120211150148.patch text/plain 2901 noel.gordon U3VidmVyc2lvbiBSZXZpc2lvbjogMTA3NDU1CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZTUzZDYzMzFhZmI2ZGQxZGQ5ZmQ3YTY5OWZmMGM2ZjNj NTY0NDcxNi4uNjY2ZWNlZTVkOGFkZGU2ZDAzYTMyNzExZmFhZjVlYzc2YTFjZDU1YyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1 IEBACisyMDEyLTAyLTEwICBOb2VsIEdvcmRvbiAgPG5vZWwuZ29yZG9uQGdtYWlsLmNvbT4KKwor ICAgICAgICB3ZWJraXQtcGF0Y2ggdXBsb2FkICJwcmV0dHkgZGlmZiIgc2hvdWxkIHdvcmsgaW4g Y3lnd2luCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03 ODQwOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog U2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL2NvbmZpcm1kaWZmLnB5OgorICAgICAgICAoQ29u ZmlybURpZmYuX3Nob3dfcHJldHR5X2RpZmYpOiAgRGV0ZWN0IGN5Z3dpbiBlbnZpcm9tZW50IGFu ZCBzcGF3biBhbiBvcGVuIGNvbW1hbmQKKyAgICAgICAgd2l0aCB0aGUgZGlmZiBmaWxlIHVybCBh cyBhbiBhcmd1bWVudCB0byBvcGVuIHRoZSBmaWxlIGluIHRoZSBkZWZhdWx0IHZpZXdlciBmb3Ig dGhlCisgICAgICAgIGZpbGUgdHlwZSAoLmh0bWwpLgorCiAyMDEyLTAyLTEwICBBZGFtIEtsZWlu ICA8YWRhbWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEVuYWJsZSBNVVRBVElPTl9PQlNFUlZF UlMgYnkgZGVmYXVsdCBvbiBhbGwgcGxhdGZvcm1zCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L3Rvb2wvc3RlcHMvY29uZmlybWRpZmYucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtp dHB5L3Rvb2wvc3RlcHMvY29uZmlybWRpZmYucHkKaW5kZXggODZjOGEyYzhkMmE4YmIxN2ZhNWRi ZjhiMTcxOGI4YWE0MzBkYTJlMi4uZDY4ODJhN2VlYTRhMWEyNGJmYjYyNThhNGZjMjYwZTMzNWJh OTA1ZCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL2NvbmZp cm1kaWZmLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9zdGVwcy9jb25maXJt ZGlmZi5weQpAQCAtMjYsNiArMjYsOCBAQAogIyAoSU5DTFVESU5HIE5FR0xJR0VOQ0UgT1IgT1RI RVJXSVNFKSBBUklTSU5HIElOIEFOWSBXQVkgT1VUIE9GIFRIRSBVU0UKICMgT0YgVEhJUyBTT0ZU V0FSRSwgRVZFTiBJRiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4K IAoraW1wb3J0IGNvbW1hbmRzCitpbXBvcnQgcGxhdGZvcm0KIGltcG9ydCB1cmxsaWIKIAogZnJv bSB3ZWJraXRweS50b29sLnN0ZXBzLmFic3RyYWN0c3RlcCBpbXBvcnQgQWJzdHJhY3RTdGVwCkBA IC00NiwxNSArNDgsMjcgQEAgY2xhc3MgQ29uZmlybURpZmYoQWJzdHJhY3RTdGVwKToKICAgICAg ICAgXQogCiAgICAgZGVmIF9zaG93X3ByZXR0eV9kaWZmKHNlbGYsIGRpZmYpOgotICAgICAgICBp ZiBub3Qgc2VsZi5fdG9vbC51c2VyLmNhbl9vcGVuX3VybCgpOgorICAgICAgICBjeWd3aW4gPSBw bGF0Zm9ybS5zeXN0ZW0oKS5zdGFydHN3aXRoKCdDWUdXSU4nKQorICAgICAgICBpZiBub3QgY3ln d2luIGFuZCBub3Qgc2VsZi5fdG9vbC51c2VyLmNhbl9vcGVuX3VybCgpOgogICAgICAgICAgICAg cmV0dXJuIE5vbmUKIAogICAgICAgICB0cnk6CiAgICAgICAgICAgICBwcmV0dHlfcGF0Y2ggPSBQ cmV0dHlQYXRjaChzZWxmLl90b29sLmV4ZWN1dGl2ZSwKICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIHNlbGYuX3Rvb2wuc2NtKCkuY2hlY2tvdXRfcm9vdCkKICAgICAgICAg ICAgIHByZXR0eV9kaWZmX2ZpbGUgPSBwcmV0dHlfcGF0Y2gucHJldHR5X2RpZmZfZmlsZShkaWZm KQotICAgICAgICAgICAgdXJsID0gImZpbGU6Ly8lcyIgJSB1cmxsaWIucXVvdGUocHJldHR5X2Rp ZmZfZmlsZS5uYW1lKQotICAgICAgICAgICAgc2VsZi5fdG9vbC51c2VyLm9wZW5fdXJsKHVybCkK KworICAgICAgICAgICAgaWYgbm90IGN5Z3dpbjoKKyAgICAgICAgICAgICAgICB1cmwgPSAiZmls ZTovLyVzIiAlIHVybGxpYi5xdW90ZShwcmV0dHlfZGlmZl9maWxlLm5hbWUpCisgICAgICAgICAg ICAgICAgc2VsZi5fdG9vbC51c2VyLm9wZW5fdXJsKHVybCkKKyAgICAgICAgICAgIGVsc2U6Cisg ICAgICAgICAgICAgICAgIyBTcGF3biB3aW4zMiBvcGVuIGNvbW1hbmQgd2l0aCBhIHVybCBhcyBh cmd1bWVudCB0byBvcGVuIHRoZQorICAgICAgICAgICAgICAgICMgZmlsZSBpbiB0aGUgZGVmYXVs dCB2aWV3ZXIgZm9yIHRoZSBmaWxlIHR5cGUgKGh0bWwpLgorICAgICAgICAgICAgICAgIHJvb3Qg PSBjb21tYW5kcy5nZXRvdXRwdXQoImN5Z3BhdGggLWEgLW0gLyIpCisgICAgICAgICAgICAgICAg dXJsID0gImZpbGU6Ly8vJXMiICUgcm9vdCArIHVybGxpYi5xdW90ZShwcmV0dHlfZGlmZl9maWxl Lm5hbWUpCisgICAgICAgICAgICAgICAgc3RhdHVzID0gY29tbWFuZHMuZ2V0c3RhdHVzb3V0cHV0 KCdjbWQuZXhlIC9jICJzdGFydCAlcyInICUgdXJsKQorICAgICAgICAgICAgICAgIGlmIHN0YXR1 c1swXTogICMgU2hvdyBlcnJvciByZWFzb24gKGNvbW1hbmQgb3V0cHV0KS4KKyAgICAgICAgICAg ICAgICAgICAgcHJpbnQgc3RhdHVzWzFdCisKICAgICAgICAgICAgICMgV2UgcmV0dXJuIHRoZSBw cmV0dHlfZGlmZl9maWxlIGhlcmUgYmVjYXVzZSB3ZSBuZWVkIHRvIGtlZXAgdGhlCiAgICAgICAg ICAgICAjIGZpbGUgYWxpdmUgdW50aWwgdGhlIHVzZXIgaGFzIGhhZCBhIGNoYW5jZSB0byBjb25m aXJtIHRoZSBkaWZmLgogICAgICAgICAgICAgcmV0dXJuIHByZXR0eV9kaWZmX2ZpbGUK 126654 2012-02-11 15:42:45 -0800 2012-02-11 16:39:51 -0800 Patch bug-78408-20120212104243.patch text/plain 2318 noel.gordon U3VidmVyc2lvbiBSZXZpc2lvbjogMTA3NDU1CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZTUzZDYzMzFhZmI2ZGQxZGQ5ZmQ3YTY5OWZmMGM2ZjNj NTY0NDcxNi4uMWZmY2I0YzhmMDRiNGVjMGRiZGNmZjNlZTk3Zjg4OTExYjM2MGEzYyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0 IEBACisyMDEyLTAyLTExICBOb2VsIEdvcmRvbiAgPG5vZWwuZ29yZG9uQGdtYWlsLmNvbT4KKwor ICAgICAgICB3ZWJraXQtcGF0Y2ggdXBsb2FkICJwcmV0dHkgZGlmZiIgc2hvdWxkIHdvcmsgaW4g Y3lnd2luCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03 ODQwOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog U2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL3VzZXIucHk6CisgICAgICAgIChVc2VyLmNh bl9vcGVuX3VybCk6IEFsbG93IGN5Z3dpbiB0byBvcGVuIHVybHMuCisgICAgICAgIChVc2VyLm9w ZW5fdXJsKTogUnVuIGRlZmF1bHQgb3BlbiBjb21tYW5kIGZvciB1cmxzIGluIGN5Z3dpbiBjYXNl LgorCiAyMDEyLTAyLTEwICBBZGFtIEtsZWluICA8YWRhbWtAY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIEVuYWJsZSBNVVRBVElPTl9PQlNFUlZFUlMgYnkgZGVmYXVsdCBvbiBhbGwgcGxhdGZvcm1z CmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vdXNlci5w eSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS91c2VyLnB5CmluZGV4IDMx YjIxOGMyZjU4MThhNzAzM2YxOWYyOWE2OThhMGVjNzBhMTEzZDEuLjA0Y2QxNTI3ZGQyZTY1OTY4 ZWQ0ZjQ2YzM2MWMyNjdmY2IwN2I3YjkgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0 cHkvY29tbW9uL3N5c3RlbS91c2VyLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29t bW9uL3N5c3RlbS91c2VyLnB5CkBAIC0yNiw5ICsyNiwxMSBAQAogIyAoSU5DTFVESU5HIE5FR0xJ R0VOQ0UgT1IgT1RIRVJXSVNFKSBBUklTSU5HIElOIEFOWSBXQVkgT1VUIE9GIFRIRSBVU0UKICMg T0YgVEhJUyBTT0ZUV0FSRSwgRVZFTiBJRiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBT VUNIIERBTUFHRS4KIAoraW1wb3J0IGNvbW1hbmRzCiBpbXBvcnQgZ2V0cGFzcwogaW1wb3J0IGxv Z2dpbmcKIGltcG9ydCBvcworaW1wb3J0IHBsYXRmb3JtCiBpbXBvcnQgcmUKIGltcG9ydCBzaGxl eAogaW1wb3J0IHN1YnByb2Nlc3MKQEAgLTE0MCw5ICsxNDIsMTkgQEAgY2xhc3MgVXNlcihvYmpl Y3QpOgogICAgICAgICAgICAgd2ViYnJvd3Nlci5nZXQoKQogICAgICAgICAgICAgcmV0dXJuIFRy dWUKICAgICAgICAgZXhjZXB0IHdlYmJyb3dzZXIuRXJyb3IsIGU6Ci0gICAgICAgICAgICByZXR1 cm4gRmFsc2UKKyAgICAgICAgICAgIHJldHVybiBwbGF0Zm9ybS5zeXN0ZW0oKS5zdGFydHN3aXRo KCdDWUdXSU4nKQogCiAgICAgZGVmIG9wZW5fdXJsKHNlbGYsIHVybCk6Ci0gICAgICAgIGlmIG5v dCBzZWxmLmNhbl9vcGVuX3VybCgpOgotICAgICAgICAgICAgX2xvZy53YXJuKCJGYWlsZWQgdG8g b3BlbiAlcyIgJSB1cmwpCi0gICAgICAgIHdlYmJyb3dzZXIub3Blbih1cmwpCisgICAgICAgIGlm IG5vdCBwbGF0Zm9ybS5zeXN0ZW0oKS5zdGFydHN3aXRoKCdDWUdXSU4nKToKKyAgICAgICAgICAg IGlmIG5vdCBzZWxmLmNhbl9vcGVuX3VybCgpOgorICAgICAgICAgICAgICAgIF9sb2cud2Fybigi RmFpbGVkIHRvIG9wZW4gJXMiICUgdXJsKQorICAgICAgICAgICAgd2ViYnJvd3Nlci5vcGVuKHVy bCkKKyAgICAgICAgICAgIHJldHVybgorCisgICAgICAgIGlmIHVybC5zdGFydHN3aXRoKCJmaWxl Oi8vLyIpOgorICAgICAgICAgICAgY3lnd2luX3Jvb3QgPSBjb21tYW5kcy5nZXRvdXRwdXQoImN5 Z3BhdGggLWEgLW0gLyIpCisgICAgICAgICAgICB1cmwgPSB1cmwucmVwbGFjZSgiZmlsZTovLy8i LCAiZmlsZTovLy8lcy8iICUgY3lnd2luX3Jvb3QsIDEpCisgICAgICAgIG9wZW5fZGVmYXVsdF92 aWV3ZXJfY29tbWFuZCA9ICdjbWQuZXhlIC9jICJzdGFydCAlcyInICUgdXJsCisgICAgICAgIHN0 YXR1cyA9IGNvbW1hbmRzLmdldHN0YXR1c291dHB1dChvcGVuX2RlZmF1bHRfdmlld2VyX2NvbW1h bmQpCisgICAgICAgIGlmIHN0YXR1c1swXTogICMgU2hvdyBlcnJvciByZWFzb24gKGNvbW1hbmQg b3V0cHV0KS4KKyAgICAgICAgICAgIHByaW50IHN0YXR1c1sxXQo=