Error handler/helper in IIS is not output but returns CGI error due to missing headers

[ryano@…]

To work with IIS 6 under Windows Server 2003 the trac.cgi file needs an additional two lines to set the content type headers otherwise IIS will report the following error;

CGI Error
The specified CGI application misbehaved by not returning a complete set of HTTP headers.

The relevant portion of the trac.cgi file was changed from this;

    # PATCH IIS6 - start
    import os
    os.environ['TRAC_ENV'] = "mypath"
    # PATCH IIS6 - end

to this;

    # PATCH IIS6 - start
    import os
    print 'Content-type: text/html'
    print
    os.environ['TRAC_ENV'] = "mypath"
    # PATCH IIS6 - end

Doing this enables the script to work under IIS 6 on Windows Server 2003.

Not sure if this is by design as I am new to the trac system but thought it would help others.

Ryan

[anonymous]

Apologies, the error actually occurred because the line;

os.environ['TRAC_ENV'] = "mypath"

was not pointing at a correct path. The error handler then tried to output helpful text which did not appear due to IIS not receiving the headers. Therefore the real fix is to move the lines;

    print 'Content-type: text/html'
    print

to the error handling code so that it is displayed to IIS users.

[markus]

Looks like a regression. I think this has already been fixed some months (years?) ago, after I had a chat with cmlenz.

[Remy Blank]

Is this still a problem in 0.11.1?

[Remy Blank]

I can still reproduce this issue in the case where TRAC_ENV is not set correctly, as it triggers an IOError exception in open_environment() that is not caught anywhere.

[Remy Blank]

Patch against 0.11-stable catching all exceptions while opening the environment

[Remy Blank]

The patch above catches all exceptions while opening the environment, not only TracError. This ensures that a correct error page is displayed even if the environment is not configured correctly, not only with CGI, but also e.g. with tracd.

Comments welcome.

[Christian Boos]

The patch is fine.

A 'message': to_unicode(e.detail) would even be better than a simple 'message': e.detail, as that would call __unicode__ on TracErrors and force a safe conversion to unicode on other Exceptions.

[Remy Blank]

Thanks for the review. Patch and suggestion applied in [7778].

[Christian Boos]

Oops, review incomplete ;-) I've just seen that type was supposed to be either 'TracError' or 'internal', as you can see in the <py:choose test="type"> in trac/template/error.html. Can you check?

As for seeing the actual class name of the exception, to_unicode would take care of this (see patch attachment:t7935-log-exception-unicode-r7778.diff:ticket:7935​). An alternative would be to replace the <py:when test="'internal'"> condition with a <py:otherwise>.

[Remy Blank]

Right, good catch. I had erroneously assumed that data['type'] was intended to be the exception type, without checking the error template.

I'm not sure if having to_unicode() prepend the exception type is a good idea, as you most likely don't want that to happen for TracError and derived classes, for example. I tried it (with a check for TracError), and all permission errors started getting a PermissionError: prefix, as PermissionError derives from StandardError and not TracError (any idea why this is the case, BTW?).

IMO, to_unicode(e) should behave mostly like str(e), which doesn't prepend the exception name. I have fixed [7778] to leave type as 'TracError', and prepend the exception name to message if it is not a TracError. This is consistent with the processing of other exceptions in _dispatch_request(). Done in [7782].

[Christian Boos]

Replying to rblank:

I'm not sure if having to_unicode() prepend the exception type is a good idea, as you most likely don't want that to happen for TracError and derived classes, for example. I tried it (with a check for TracError), and all permission errors started getting a PermissionError: prefix,

Ok

as PermissionError derives from StandardError and not TracError (any idea why this is the case, BTW?).

No good reason I guess. Before changing it, we should verify the try/catch clauses. The one I had in mind (in source:trunk/trac/web/main.py​) already catches PermissionError before TracError, so we would be safe there.

[Christian Boos]

Not yet there … just seen a regression on #5066 that can be traced to r7782.

Actually e.detail can't even be a TracError, see HTTPException constructor in source:trunk/trac/web/api.py​.

Ok with the following fix?

trac/web/main.py

@@ -42 +42 @@
                       arity
 from trac.util.compat import partial, reversed
 from trac.util.datefmt import format_datetime, http_date, localtz, timezone
-from trac.util.text import shorten_line, to_unicode
+from trac.util.text import shorten_line, to_unicode, exception_to_unicode
 from trac.util.translation import _
 from trac.web.api import *
 from trac.web.chrome import Chrome
@@ -443 +443 @@
                 title = e.reason
             else:
                 title = 'Error: %s' % e.reason
-        message = to_unicode(e.detail)
-        if isinstance(e.detail, Exception) and \
-                not isinstance(e.detail, TracError):
-            message = '%s: %s' % (e.detail.__class__.__name__, message)
+        message = e.detail
+        if isinstance(e.detail, Exception):
+            # Note that e.detail can't be a TracError, see HTTPException
+            message = exception_to_unicode(e.detail)
         data = {'title': title, 'type': 'TracError', 'message': message,
                 'frames': [], 'traceback': None}
         if e.code == 403 and req.authname == 'anonymous':

trac/util/text.py

@@ -63 +63 @@
         except UnicodeError:
             return unicode(text, locale.getpreferredencoding(), 'replace')
 
+def exception_to_unicode(e):
+    return '%s: %s' % (e.__class__.__name__, to_unicode(e))
+
 def javascript_quote(text):
     """Quote strings for inclusion in javascript"""
     return text.replace('\\', '\\\\').replace('\r', '\\r') \

(exception_to_unicode to be reused in various places where we forgot to convert exception to unicode, e.g. #7935)

[Remy Blank]

Replying to cboos:

Not yet there … just seen a regression on #5066 that can be traced to r7782.

Argh… All this because I wanted to see the type of the exception. Without that, fixing this ticket would have been a one-liner. Oh well…

Actually e.detail can't even be a TracError, see HTTPException constructor in source:trunk/trac/web/api.py​.

I had actually noticed this in my logs, but failed to make the connection.

Ok with the following fix?

Looks good. Applied in [7794]. Thanks!

(One question, though. How on earth did you catch the regression on #5066?)

[Christian Boos]

Replying to rblank:

(One question, though. How on earth did you catch the regression on #5066?)

Well, exactly like the error on #5066 shows, by trying to browse directly /some/path by (mis)typing "/some/pathx" in the search box.

An actual functional test case would be great, for sure.