Edgewall Software
Modify

Opened 9 years ago

Closed 20 months ago

Last modified 20 months 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
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:

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 8 years ago.
patch against 0.12-stable r10370
log-perm-8790-vs-trunk-10771.patch (919 bytes ) - added by Mikhail Terekhov <termim@…> 7 years ago.
another patch against trunk r10771

Download all attachments as: .zip

Change History (32)

comment:1 Changed 9 years ago by anonymous

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

comment:2 Changed 9 years ago by anonymous

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 Changed 9 years ago by martin@…

Cc: martin@… added

comment:2 is me, forgot my email address.

comment:4 Changed 9 years ago by Tim Hatch

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

Eli, we should make a test for this.

comment:5 Changed 9 years ago by Ryan Ollos <ryano@…>

Cc: ryano@… added

comment:6 Changed 9 years ago by Carsten Klein <carsten.klein@…>

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 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Priority: normalhigh
Severity: blockercritical

Changed 8 years ago by Thijs Triemstra

Attachment: log-perm-8790.patch added

patch against 0.12-stable r10370

comment:8 Changed 8 years ago by Thijs Triemstra

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 Changed 8 years ago by Remy Blank

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 Changed 8 years ago by Remy Blank

Keywords: needfixup added

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

comment:11 Changed 8 years ago by anonymous

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 Changed 8 years ago by Remy Blank

Milestone: 0.12.2next-major-0.1X

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

Changed 7 years ago by Mikhail Terekhov <termim@…>

another patch against trunk r10771

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

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

Could we please move it to 0.13?

comment:14 Changed 7 years ago by Christian Boos

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 Changed 6 years ago by Christian Boos

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 Changed 4 years ago by Ryan J Ollos

Cc: Ryan J Ollos added; ryano@… removed

comment:18 Changed 4 years ago by Ryan J Ollos

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 Changed 4 years ago by Ryan J Ollos

Owner: Remy Blank deleted

comment:20 Changed 3 years ago by Ryan J Ollos

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

comment:21 Changed 20 months ago by Ryan J Ollos

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 20 months ago by Ryan J Ollos (previous) (diff)

comment:22 Changed 20 months ago by Ryan J Ollos

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 Changed 20 months ago by Ryan J Ollos

Milestone: 1.3.21.2.2

comment:24 Changed 20 months ago by Ryan J Ollos

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 Changed 20 months ago by Ryan J Ollos

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

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

comment:26 Changed 20 months ago by Jun Omae

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 20 months ago by Jun Omae (previous) (diff)

comment:27 Changed 20 months ago by Ryan J Ollos

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

comment:28 Changed 20 months ago by Ryan J Ollos

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

comment:29 Changed 20 months ago by Jun Omae

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

comment:30 Changed 20 months ago by Ryan J Ollos

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 20 months ago by Ryan J Ollos (previous) (diff)

comment:31 Changed 20 months ago by Ryan J Ollos

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.
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.