Multiple webbrowser.py bug fixes / improvements

[sdeibel]

BPO	728278
Nosy	@mwhudson, @loewis, @freddrake, @birkenfeld, @phdru
Files	wingwebbrowser.tgz: Same as previous upload with corrected unit test (oops) wingwebbrowser.py: Slightly updated file w/ fix for empty ("") BROWSER env

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/freddrake'
closed_at = <Date 2005-09-15.08:04:05.000>
created_at = <Date 2003-04-27.04:02:26.000>
labels = ['library']
title = 'Multiple webbrowser.py bug fixes / improvements'
updated_at = <Date 2005-09-15.08:04:05.000>
user = 'https://bugs.python.org/sdeibel'

bugs.python.org fields:

activity = <Date 2005-09-15.08:04:05.000>
actor = 'georg.brandl'
assignee = 'fdrake'
closed = True
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2003-04-27.04:02:26.000>
creator = 'sdeibel'
dependencies = []
files = ['5216', '5217']
hgrepos = []
issue_num = 728278
keywords = ['patch']
message_count = 13.0
messages = ['43493', '43494', '43495', '43496', '43497', '43498', '43499', '43500', '43501', '43502', '43503', '43504', '43505']
nosy_count = 7.0
nosy_names = ['mwh', 'loewis', 'fdrake', 'georg.brandl', 'sdeibel', 'phd', 'rodsenra']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue728278'
versions = ['Python 2.4']

[sdeibel]

In using webbrowser.py we uncovered and fixed a number
of problems and made some improvements in usability and
consistency of behavior.

Appended below is a summary of the changes made. This
list is also found at the top of the uploaded file,
which was based on the version of webbrowser.py found
in Python 2.3a2. Sorry to submit so many changes at
once but they were all made in a period of a few days
when we went through this module for use in Wing IDE.

I'm also submitting some unit tests for the module.

I've tried to review everything carefully, further
review is definately needed. Feel free to contact me
if you have questions or comments or want me to make
changes and resubmit.

Hope this is helpful.

• Stephan

[sdeibel]

Logged In: YES
user_id=12608

Oops, correcting the upload

[sdeibel]

Logged In: YES
user_id=12608

Jeez, here is the list of changes in this patch which I
meant to append to the original report.

Bugs fixed:

• Don't apply lower() to command lines or commands that are
  going to be executed
• Don't confuse browser name/id with command line used to
  launch the browser
• Require that browser commands be executable
• Identify user-provided strings as a command line by
  looking for any args and
  not just for %s
• Handle spaces in user-provided command names, either if
  quoted on the
  user-provided command line or escaped with back slashes
• Use '%s' instead of "%s", which avoids character
  interpretation on
  Unix command lines
• Escape and quote urls more safely to prevent crafting urls
  that result
  in command lines that execute arbitrary commands
• Fixed Galeon so it doesn't hang up the app until the
  browser is quit
• Added the Mac OS X support that doesn't make bad
  environment assumptions
  on OS 10.1
• Fixed win32 use of Netscape, which before would never work
  because
  the _tryorder entry was being pruned out
• Leave found items in _browsers and _tryorder on OS X (but
  prepend
  the OS X specific support in _tryorder). OS X is Unix so
  these are useful
  there.
• Now add %s to end of command lines if they don't contain
  %s already, for
  compatibility e.g. with KDE's default value for BROWSER
  environment
• Now add '&' to end of Unix command lines that are going to
  be launched
  from GenericBrowser to avoid hanging up on user-provided
  command lines
• Added additional quoting of command names, so some of the
  browser classes
  can work with a renamed browser with a space in the
  executable name
• Added a number of return values / checks where before
  there were bad
  assumptions in the code that might have led to errors

Other internal changes made:

• Added unit tests
• Removed the loop in get() which always returned in the
  first iteration
• _tryorder more tightly coupled with registering in
  _browsers, and removed
  potentially problematic last-ditch GenericBrowser
  registering clause and
  the prune-_tryorder clauses from the end of the module
• _iscommand also returns true if cmd is an existing file
  name, and it
  returns the full path or None instead of just True and False
• _synthesize registers and returns GenericBrowser if
  synthesis fails but
  the user-provided command line looks valid
• Some additional uses of True and False instead of 1 and 0

Changes to the public interface:

• Added optional update_tryorder arg to register() (default
  value is same
  as previous action of this function)
• Open/open_new return False if command definately failed
  and True if
  it may have succeeded (both previously returned None in
  all cases)
• Get() returns a GenericCommand if 'using' is given and
  doesn't name or
  synthesize a browser (previously it did this correctly
  only when %s
  was in the given command line)
• Values passed via BROWSER environment will be registered
  and synthesized
  in the same way as 'using' arg values are treated in get()

[loewis]

Logged In: YES
user_id=21627

Because this is quite a large change, I recommend to
postpone it after 2.3.

[sdeibel]

Logged In: YES
user_id=12608

Yes, I intended it to be post-2.3. The Group popup choices
doesn't include 2.4, however.

[mwhudson]

Logged In: YES
user_id=6656

Is now.

[sdeibel]

Logged In: YES
user_id=12608

I'm uploading a newer version of the webbrowser.py file,
which contains a
small fix to avoid failing when there is an empty ("")
BROWSER environment variable set. The diff is just this:

--- wingwebbrowser.py   20 Feb 2004 23:45:26 -0000      1.3
+++ wingwebbrowser.py   10 Nov 2004 16:51:36 -0000      1.4
@@ -210,6 +210,8 @@
 
 def _splitcommand(cmd):
     """Extract command name and args from command line"""
+    if cmd == '':
+        return '', ''
     if cmd[0] in ('"', "'"):
         name = cmd[1:cmd[1:].find(cmd[0])+1]
         args = cmd[len(name)+2:].strip()
@@ -603,7 +605,8 @@
     # Treat choices in same way as if passed into get() but
do register
     # and prepend to _tryorder
     for cmdline in _userchoices:
-        _synthesize(cmdline, -1, True)
+        if cmdline != '':
+            _synthesize(cmdline, -1, True)
     cmdline = None # to make del work if _userchoices was empty
     del cmdline
     del _userchoices

[rodsenra]

Logged In: YES
user_id=9057

According to the Patch Submission Guidelines, we are
supposed to submit patch files nor modified versions of
modules nor zip files. I advise you
to resubmit according to the guidelines documented in
http://python.org/patches/

[rodsenra]

Logged In: YES
user_id=9057

Against the [http://python.org/patches/ Patch Submission
Guidelines] this entry has uploaded both the whole file and
.tgz, but no patch file.

The large numbers of changes make it difficult to review.

All the changes were documented to the top of the file, I
don't believe they
belong there. At least that is not complaint to the first
rule in
http://www.python.org/patches/style.html

There are several OS X specific corrections that I'm unable
to reproduce/validate, since I have no access to that
platform.

The modules was renamed to wingwebbrowser.py and the test
case was built using this name. I believe this is
inadequate, the original module name should be kept.

There are three categories of changes: bug fixes, internal
changes and interface changes. IMO it would be better to
break this patch in at least three, matching these types of
changes. Bug fixes would have a higher priority, less chance
to break backward compatibility and more chance to be
incorporated earlier.

It should be applied before patch 1077979, otherwise that
fix would be reversed.

In spite of the formatting comments above, there are
*valuable* changes in the submitted file.

This is a nice candidate to be tackled in a Python Bug Day,
since it functionality involves a broad range of platforms
and user browsers preferences. Moreover, thourough test
cases are hard to be cooked.

I recommend keeping it open until somebody (possibly its
author) reshape it properly and then applying it.

[sdeibel]

Logged In: YES
user_id=12608

Sorry, guilty as charged re: uploading whole files. I did
this for lack of time since there is some value here. Diffs
are also not that useful given large number of changes.
Agree this needs to be split up.

Some additional comments:

Splitting off the bug fixes for specific OSes may also be a
good idea so they can be reviewed separately.

Ediff is a good way to review the changes against current
sources and make smaller patches.

I deleted the initial (bad) upload. Not sure why I didn't
do that originally.

[phdru]

Logged In: YES
user_id=4799

After two days of studying and playing with the patch I
recommend to reject it. I can draw one or two smaller things
from it - _isexecutable(), for example. But the entire patch
is too big, too invasive, and sometimes does more harm than
good.

The problem number one. The patch tries to emulate a Unix
shell - it splits command-line into words, it escapes shell
metacharacters. And of course it fails. Nobody can emulate a
shell, because there is no THE shell. There are far too many
shells, all different. The patch escapes backslash, dollar
sign, backtick and semicolon. What about other special
characters, '!', '|', or asterisk?

The problem number two. It adds ' &' to the every
GenericBrowser command line. That's bad for text-mode
console browsers. If I define BROWSER="lynx %s" I certainly
don't want to run it in the background. And that's
incompatible with the patch 754022 that runs browsers in
_tryorder to find one that works; running a browser in the
background prevents check if the browser has been started.

The problem number three. The patch quotes too much. It
quotes %s in remote commands: instead of
openURL(http://example.com) it runs
openURL('http://example.com'). Mozilla does not accept it
and report error 509. (I don't know which instance rejects
it - current or remote.)

[phdru]

Logged In: YES
user_id=4799

I'd like to return to discuss some parts that I've
recommended to reject. When I've said "do not try to emulate
a shell" I didn't mean "never manipulate a command-line", I
meant "never write your own shell". Do not parse a
command-line string, do not split it into words, do not
interpret these words, quote them, combine and pass them to
the real OS shell.

Instead of manipulating strings manipulate lists (of strings
and markers). Instead of quoting special characters avoid
using shell altogether - use fork+exec (or spawn). This is
much more safe.

That said, I'd like to ask you to rewrite _safequote() and
patch the entire webbrowser.py based on these ideas.

[birkenfeld]

Logged In: YES
user_id=1188172

Relevant ideas are now in bpo-754022.