Edgewall Software
Modify

Opened 8 years ago

Closed 12 months ago

#11538 closed defect (fixed)

Duplicate options are allowed in sections of trac.ini

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.5.3
Component: general Version:
Severity: normal Keywords: config python3
Cc: Branch:
Release Notes:

ConfigurationError will be raised when there are duplicate sections and options in trac.ini.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

The sections of trac.ini allow duplicate options, with later values overwriting those that come first. Two patterns that could lead to confusing behavior when the user doesn't realize that the option's value is being changed later in the file are:

  1. Duplicate sections with the same option.
    [trac]
    default_handler = WikiModule
    
    ...
    
    [trac]
    default_handler = TicketModule
    
  2. Duplicate options within a section.
    [trac]
    default_handler = WikiModule
    
    ...
    
    default_handler = TicketModule
    

While Python 3.x has a much improved configparser module with a strict mode that raises an error when an option appears more than once, I'm not sure there is much we can do in Python 2.x using ConfigParser.

SO:5396653 suggests using a dict_type that allows multiple values with the same key, however dict_type is only supported in Python 2.6, so we wouldn't be able to apply as dict_type-based fix to 1.0-stable.

Attachments (0)

Change History (11)

comment:1 by Ryan J Ollos, 8 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 7 years ago

Keywords: python3 added

After #11982, which replaced use of ConfigObj with ConfigParser, an exception is no longer raised when there is a duplicate section or option in an authz_policy configuration file. One test is commented out: trunk/tracopt/perm/tests/authz_policy.py@13994:223-234#L223.

comment:3 by Ryan J Ollos, 20 months ago

Milestone: undecided1.5.2

comment:5 by Ryan J Ollos, 19 months ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:6 by Ryan J Ollos, 12 months ago

Milestone: 1.5.21.5.3

I think this could avoid some difficult to find problems. Examples:

Internal Server Error

TracError: DuplicateSectionError: While reading from '/Users/rjollos/Documents/Workspace/trac-dev/tracenvs/proj-1.5/conf/trac.ini' [line  8]: section 'attachment' already exists
Internal Server Error

TracError: DuplicateOptionError: While reading from '/Users/rjollos/Documents/Workspace/trac-dev/tracenvs/proj-1.5/conf/trac.ini' [line 39]: option 'acct_mgr.register.*' in section 'components' already exists

Proposed changes in [00a4344992/rjollos.git]. Deferring to 1.5.3. Please let me know if you see any major problems with switching to strict parsing.

Last edited 12 months ago by Ryan J Ollos (previous) (diff)

comment:7 by Jun Omae, 12 months ago

I consider AuthzPolicy.parse_authz() should catch all Errors of configparser when parsing the file.

         self.authz = UnicodeConfigParser(ignorecase_option=False)
         try:
             self.authz.read(self.authz_file)
-        except ParsingError as e:
+        except configparser.Error as e:
             self.log.error("Error parsing authz permission policy file: %s",
                            exception_to_unicode(e))
-            raise ConfigurationError()
+            raise ConfigurationError(_("Error parsing authz permission policy "
+                                       "file: %(err)s",
+                                       err=exception_to_unicode(e)))
Last edited 12 months ago by Jun Omae (previous) (diff)

comment:8 by Ryan J Ollos, 12 months ago

Catching all Errors is a good idea. Thanks.

When working on this module previously I intentionally didn't raise a message for ConfigurationError because it seemed like a possible security risk.

comment:9 by Ryan J Ollos, 12 months ago

Also noticed #13335, which I'll fix on 1.4-stable.

comment:10 by Ryan J Ollos, 12 months ago

Release Notes: modified (diff)

Committed to trunk in r17491.

comment:11 by Ryan J Ollos, 12 months ago

Resolution: fixed
Status: assignedclosed

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.