Edgewall Software
Modify

Opened 14 years ago

Closed 7 years ago

Last modified 7 years ago

#8790 closed defect (fixed)

[PATCH] trac becomes unusable after entering a log file path without proper permissions

Reported by: bernardfrancois Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.2
Component: general Version: 0.11.1
Severity: normal Keywords: patch log bitesized
Cc: martin@…, retracile@…, Thijs Triemstra, Ryan J Ollos Branch:
Release Notes:

Invalid logging configuration can no longer disable the Trac instance when saving the configuration through the WebAdmin. Invalid configuration changes are not saved and a warning is displayed with the exception message.

API Changes:
Internal Changes:

Description

If you enter a log file path without proper permissions, trac totally breaks. You get an 'Internal Server Error' message on every page.

The only remedy I found is by changing the path again in the trac.ini file.

I reported this bug in the ubuntu launchpad:

https://bugs.launchpad.net/ubuntu/+source/trac/+bug/469641

Attachments (2)

log-perm-8790.patch (864 bytes ) - added by Thijs Triemstra 13 years ago.
patch against 0.12-stable r10370
log-perm-8790-vs-trunk-10771.patch (919 bytes ) - added by Mikhail Terekhov <termim@…> 13 years ago.
another patch against trunk r10771

Download all attachments as: .zip

Change History (32)

comment:1 by anonymous, 14 years ago

Note: for example, enter '/trac.log' as log file path, and see what happens…

comment:2 by anonymous, 14 years ago

I second that bug. I'm using Trac 0.11.5 under Ubuntu 9.10.

With:

[logging]
log_type = file
log_level = DEBUG 
lof_file = /some/path/trac/doesnt/have/permission/to

gives:

Internal Server Error

TracError: IOError: (13, 'Permission denied')

IMHO, Trac should be more forgiving in this case of misconfiguration.

comment:3 by martin@…, 14 years ago

Cc: martin@… added

comment:2 is me, forgot my email address.

comment:4 by Tim Hatch, 14 years ago

Cc: retracile@… added
Milestone: next-minor-0.12.x

Eli, we should make a test for this.

comment:5 by Ryan Ollos <ryano@…>, 14 years ago

Cc: ryano@… added

comment:6 by Carsten Klein <carsten.klein@…>, 14 years ago

I believe that the main problem lies in log.FileHandler#_open, which uncoditionally tries to open the file.

However, I'd rather display a more detailed error message to the user than simply ignoring the fact that logging was disabled.

So, basically in trac.web.main#_dispatch_request(…)

    try:
        if not env and env_error:
            raise HTTPInternalError(env_error)

The error should be made more clear to the user.

Don't know if this would require additional work on env.Environment#setup_log() where it will create the log file.

comment:7 by Thijs Triemstra, 13 years ago

Cc: Thijs Triemstra added
Priority: normalhigh
Severity: blockercritical

by Thijs Triemstra, 13 years ago

Attachment: log-perm-8790.patch added

patch against 0.12-stable r10370

comment:8 by Thijs Triemstra, 13 years ago

Keywords: patch added
Milestone: next-minor-0.12.x0.12.2
Summary: trac becomes unuseable after entering a log file path without proper permissions[PATCH] trac becomes unuseable after entering a log file path without proper permissions

Attached a patch that prints a more descriptive error when a log file can't be saved:

Error

TracError: Permissions error for Trac log file. Unable to write: /etc/trac.log

comment:9 by Remy Blank, 13 years ago

Owner: set to Remy Blank

That's a first step. However, your access constant is wrong, I believe. The correct test would be:

if not os.access(logfile, os.F_OK | os.W_OK):

I wonder if it would be worth allowing the environment to work even without a log file. That is, instead of raising an exception, create a null handler instead of the FileHandler. Bonus points if all web accesses by a TRAC_ADMIN then display a warning explaining the issue, and the same with trac-admin calls.

Putting on my todo list to already apply the simple solution.

comment:10 by Remy Blank, 13 years ago

Keywords: needfixup added

There's another issue with this patch: it doesn't allow the initial creation of the log file.

comment:11 by anonymous, 13 years ago

Backlink to #9914: A wrong path because of using slash instead backslash makes trac unusable until the admin fixes the path in trac.ini. The webinterface input handler must be robust enough to stand such misconfiguration.

comment:12 by Remy Blank, 13 years ago

Milestone: 0.12.2next-major-0.1X

This shouldn't block 0.12.2. We can move it back when the patch is updated.

by Mikhail Terekhov <termim@…>, 13 years ago

another patch against trunk r10771

comment:13 by Mikhail Terekhov <termim@…>, 13 years ago

Somewhat simpler variant of this simple patch against current trunk (r10771).

Could we please move it to 0.13?

comment:14 by Christian Boos, 13 years ago

I wouldn't use i18n here, it's too low-level. Also, in case of such a failure we should try to setup a fallback logger on stderr.

comment:15 by Christian Boos, 12 years ago

Keywords: needfixup removed
Milestone: next-major-releasesnext-stable-1.0.x

What about this?

  • trac/env.py

     
    636636                     .replace('%(path)s', self.path) \
    637637                     .replace('%(basename)s', os.path.basename(self.path)) \
    638638                     .replace('%(project)s', self.project_name)
    639         self.log, self._log_handler = logger_handler_factory(
    640             logtype, logfile, self.log_level, logid, format=format)
     639        try:
     640            log_and_handler = logger_handler_factory(
     641                logtype, logfile, self.log_level, logid, format=format)
     642        except Exception, e:
     643            log_and_handler = logger_handler_factory(
     644                'stderr', '', self.log_level, logid, format=format)
     645            log_and_handler[0].error("Error creating the %s log handler (%s), "
     646                                     "falling back to stderr",
     647                                     logtype, exception_to_unicode(e))
     648            self.config.set('logging', 'log_type', 'stderr')
     649        self.log, self._log_handler = log_and_handler
    641650        from trac import core, __version__ as VERSION
    642651        self.log.info('-' * 32 + ' environment startup [Trac %s] ' + '-' * 32,
    643652                      get_pkginfo(core).get('version', VERSION))

comment:17 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos added; ryano@… removed

comment:18 by Ryan J Ollos, 9 years ago

Keywords: log bitesized added
Milestone: next-stable-1.0.xnext-dev-1.1.x
Priority: highnormal
Severity: criticalnormal

The patch in comment:15 looks okay to me. We might consider being more specific about the exceptions we trap though.

comment:19 by Ryan J Ollos, 9 years ago

Owner: Remy Blank removed

comment:20 by Ryan J Ollos, 8 years ago

Milestone: next-dev-1.1.xnext-dev-1.3.x

comment:21 by Ryan J Ollos, 7 years ago

Milestone: next-dev-1.3.x1.3.2
Owner: set to Ryan J Ollos
Status: newassigned

With this ticket having been open for so long, I'm going to take the approach that an imperfect solution is better than no solution. The behavior can always be further improved later on.

Proposed change in [93c1700f/rjollos.git]. Most of all, I just want to make sure that a configuration change through the Admin Logging page does not make the Trac instance unusable. I think that's unacceptable behavior.

Additionally, I considered showing the exception as a warning for admin users:

  • trac/env.py

    diff --git a/trac/env.py b/trac/env.py
    index 3ce3deeb6..02f70011c 100644
    a b class Environment(Component, ComponentManager):  
    236236        self.path = os.path.normpath(os.path.normcase(path))
    237237        self.log = None
    238238        self.config = None
     239        self.logger_exception = None
    239240
    240241        if create:
    241242            self.create(options)
    class Environment(Component, ComponentManager):  
    630631        except Exception as e:
    631632            self.log = create_logger('stderr')
    632633            self.log.error("Logger configuration error: %s", e)
     634            self.logger_exception = e
    633635        self.log.info('-' * 32 + ' environment startup [Trac %s] ' + '-' * 32,
    634636                      self.trac_version)
    635637
  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index 095b37f99..060392e64 100644
    a b class RequestDispatcher(Component):  
    190190        self.log.debug('Dispatching %r', req)
    191191        chrome = Chrome(self.env)
    192192
     193        if self.env.logger_exception and 'TRAC_ADMIN' in req.perm:
     194            add_warning(req, _("Logger configuration error: %(error)s",
     195                               error=self.env.logger_exception))
     196
    193197        try:
    194198            # Select the component that should handle the request
    195199            chosen_handler = None

Alternatively, we could only show the error when the path is /admin/general/logging. We could also print the warning when trac-admin is called. Is it worth the extra complexity?

At a minimum, I just want to get a fix committed so that the Trac instance can't be made unusable by a configuration change from the Logging Admin page. Particularly since I will propose changes for #7820 soon, and without the fix there will be more options that can lead to a mis-configured logger that can make the Trac instance unusable.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:22 by Ryan J Ollos, 7 years ago

Summary: [PATCH] trac becomes unuseable after entering a log file path without proper permissions[PATCH] trac becomes unusable after entering a log file path without proper permissions

comment:23 by Ryan J Ollos, 7 years ago

Milestone: 1.3.21.2.2

comment:24 by Ryan J Ollos, 7 years ago

I gave it some more thought. If you edit trac.ini, then your configuration is applied and you can fix it by editing trac.ini. However, if you make changes from the admin panel, we first test whether those changes are a valid configuration before allowing the changes to be applied. This prevents the Trac instance from being made unreachable by the configuration changes: [88a7ebde8/rjollos.git]. I'll add more tests before committing.

comment:25 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.2-stable in r15783, merged to trunk in r15784.

comment:26 by Jun Omae, 7 years ago

In [15783#file2], it seems that log_format = format.replace(...) should be log_format = log_format.replace(...). format is a built-in function.

        if log_format:
            log_format = format.replace('$(', '%(') \
                               .replace('%(path)s', self.path) \
Last edited 7 years ago by Jun Omae (previous) (diff)

comment:27 by Ryan J Ollos, 7 years ago

Thanks for spotting the incomplete variable rename. I added a test and will commit shortly when the CI tests pass.

comment:28 by Ryan J Ollos, 7 years ago

Fixed on 1.2-stable in r15785, merged to trunk in r15786.

comment:29 by Jun Omae, 7 years ago

After [15783], trac.log and trac.log.1 files are created by make unit-test.

comment:30 by Ryan J Ollos, 7 years ago

Thanks for spotting.

Unrelated, the call to create_logger in trac/admin/web_ui.py registers a handler if it succeeds. I wasn't worried about that because the environment is shutdown after a configuration change, destroying the logger. However I think it would be better to avoid adding the handler to the logger and to close the handler immediately after it's created. Possible fix: [efb1399d/rjollos.git].

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:31 by Ryan J Ollos, 7 years ago

comment:30 change committed to 1.2-stable in r15819, merged to trunk in r15820.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.