issue2550864 - Potential information leakage via journal/history

Fix this by making the hyperdb::Class::history function check for view
permissions on the journaled properties. So a user that sees [hidden]
for a property in the web interface doesn;t see the property changes
in the history.

While doing this, relocated the filter for quiet properties
from the templating class to the hyperdb.

Also added the skipquiet option to the history command in
roundup-admin.py to enable filtering of quiet params.

Also changed calls to history() in the backend databases to report all
items.

Changed inline documentation for all history calls that document the
actions. The create action (before nov 6 2002) used to record all
parameters. After that point the create call uses an empty dictionary.
The filtering code depends on the create dictionary being empty.
It may not operate properly on very old roundup databases.

Changed calls to logging.getLogger to roundup.hyperdb.backends to
allow filtering the back end while keeping hyperdb logging.

In cgi/templating.py, changed history() function consolidating
handiling of link and unlink actions

Added tests for quiet property filtering and permission filtering
of history.

Author: rouilj

CHANGES.txt

@@ -105,7 +105,12 @@ Features:
     prop.quiet=True
   support for anydb backend, added tests, doc updates, support for
   ignoring quiet setting using showall=True in call to history()
-  function in templates by John Rouillard.
+  function in templates by (John Rouillard). (Note implementation
+  changed while implementing fix for issue2550864. Filtering of
+  quiet properties pushed down to the hyperdb.py::Class::history
+  function. This fixes a small bug in the implementation that caused
+  a limiting the templating history call to display fewer than the
+  the requested number of items if some were quiet.)
 - issue2550767: Add newitemcopy.py detector to notify users of new
   items.  Added to detectors directory and a README.txt generated to
   describe the purpose of the directory. It also says the detectors
@@ -196,6 +201,12 @@ Features:
   genconfig but it uses values from an existing config.ini
   rather than default values. Use to update an existing
   config.ini with new options and help text. (John Rouillard)
+- issue2550864: Potential information leakage via journal/history
+  Hyperdb history function now only returns properties that the user
+  has View permissions on. Can be overridden by setting a parameter
+  when calling the method. Also restructured code that implemented
+  issue1714899 moving it from the templating class to the hyperdb.
+  (John Rouillard)
 
 Fixed:
 

doc/customizing.txt

@@ -2461,6 +2461,8 @@ history         render the journal of the current item as
                 HTML. By default properties marked as "quiet" (see 
 		`design documentation`_) are not shown unless the
 		function is called with the showall=True parameter.
+		Properties that are not Viewable to the user are not
+		shown.
 renderQueryForm specific to the "query" class - render the search form
                 for the query
 hasPermission   specific to the "user" class - determine whether the
@@ -3213,6 +3215,13 @@ journal. This is generally generated with the template::
 
  <tal:block tal:replace="structure context/history" />
 
+or::
+
+   <tal:block
+       tal:replace="structure python:context.history(showall=True)" />
+
+if you want to show history entries for quiet properties.
+
 *To be done:*
 
 *The actual history entries of the item may be accessed for manual

doc/design.txt

@@ -286,10 +286,12 @@ E.G. assuming title is part of an Issue::
 will create a property called ``title`` that will be included in the
 get_required_props() output. Calling
 db.issue.properties['title'].get_default_value() will return "not set".
-Changes to the property will not be displayed in emailed change notes,
-the history at the end of the item pages in the web interface and will
-be suppressed in the confirmation notice (displayed as a green banner)
-shown on changes.
+Changes to the property will not be displayed in:
+
+   - emailed change notes,
+   - the history at the end of the item pages in the web interface
+   - in the confirmation notice (displayed as a green banner)
+     shown on changes.
 
 These objects are used when specifying what properties belong in classes::
 

roundup/admin.py

@@ -1012,23 +1012,34 @@ def do_table(self, args):
         return 0
 
     def do_history(self, args):
-        ''"""Usage: history designator
+        ''"""Usage: history designator [skipquiet]
         Show the history entries of a designator.
 
         A designator is a classname and a nodeid concatenated,
         eg. bug1, user10, ...
 
-        Lists the journal entries for the node identified by the designator.
+        Lists the journal entries viewable by the user for the
+        node identified by the designator. If skipquiet is the
+        second argument, journal entries for quiet properties
+        are not shown.
         """
+
         if len(args) < 1:
             raise UsageError(_('Not enough arguments supplied'))
         try:
             classname, nodeid = hyperdb.splitDesignator(args[0])
         except hyperdb.DesignatorError, message:
             raise UsageError(message)
 
+        skipquiet = False
+        if len(args) == 2:
+            if args[1] != 'skipquiet':
+                raise UsageError("Second argument is not skipquiet")
+            skipquiet = True
+
         try:
-            print self.db.getclass(classname).history(nodeid)
+            print self.db.getclass(classname).history(nodeid,
+                                                      skipquiet=skipquiet)
         except KeyError:
             raise UsageError(_('no such class "%(classname)s"')%locals())
         except IndexError:

roundup/backends/back_anydbm.py

@@ -274,7 +274,7 @@ def getclass(self, classname):
     def clear(self):
         """Delete all database contents
         """
-        logging.getLogger('roundup.hyperdb').info('clear')
+        logging.getLogger('roundup.hyperdb.backend').info('clear')
         for cn in self.classes:
             for dummy in 'nodes', 'journals':
                 path = os.path.join(self.dir, 'journals.%s'%cn)
@@ -322,7 +322,7 @@ def opendb(self, name, mode):
         # whichdb() function to do this
         if not db_type or hasattr(anydbm, 'whichdb'):
             if __debug__:
-                logging.getLogger('roundup.hyperdb').debug(
+                logging.getLogger('roundup.hyperdb.backend').debug(
                     "opendb anydbm.open(%r, 'c')"%path)
             return anydbm.open(path, 'c')
 
@@ -334,7 +334,7 @@ def opendb(self, name, mode):
             raise hyperdb.DatabaseError(_("Couldn't open database - the "
                 "required module '%s' is not available")%db_type)
         if __debug__:
-            logging.getLogger('roundup.hyperdb').debug(
+            logging.getLogger('roundup.hyperdb.backend').debug(
                 "opendb %r.open(%r, %r)"%(db_type, path, mode))
         return dbm.open(path, mode)
 
@@ -395,7 +395,7 @@ def savenode(self, classname, nodeid, node):
         """ perform the saving of data specified by the set/addnode
         """
         if __debug__:
-            logging.getLogger('roundup.hyperdb').debug(
+            logging.getLogger('roundup.hyperdb.backend').debug(
                 'save %s%s %r'%(classname, nodeid, node))
         self.transactions.append((self.doSaveNode, (classname, nodeid, node)))
 
@@ -409,15 +409,15 @@ def getnode(self, classname, nodeid, db=None, cache=1):
         cache_dict = self.cache.setdefault(classname, {})
         if nodeid in cache_dict:
             if __debug__:
-                logging.getLogger('roundup.hyperdb').debug(
+                logging.getLogger('roundup.hyperdb.backend').debug(
                     'get %s%s cached'%(classname, nodeid))
                 self.stats['cache_hits'] += 1
             return cache_dict[nodeid]
 
         if __debug__:
             self.stats['cache_misses'] += 1
             start_t = time.time()
-            logging.getLogger('roundup.hyperdb').debug(
+            logging.getLogger('roundup.hyperdb.backend').debug(
                 'get %s%s'%(classname, nodeid))
 
         # get from the database and save in the cache
@@ -450,7 +450,7 @@ def destroynode(self, classname, nodeid):
         """Remove a node from the database. Called exclusively by the
            destroy() method on Class.
         """
-        logging.getLogger('roundup.hyperdb').info(
+        logging.getLogger('roundup.hyperdb.backend').info(
             'destroy %s%s'%(classname, nodeid))
 
         # remove from cache and newnodes if it's there
@@ -564,15 +564,17 @@ def addjournal(self, classname, nodeid, action, params, creator=None,
         """ Journal the Action
         'action' may be:
 
-            'create' or 'set' -- 'params' is a dictionary of property values
+            'set' -- 'params' is a dictionary of property values
+            'create' -- 'params' is an empty dictionary as of
+                      Wed Nov 06 11:38:43 2002 +0000
             'link' or 'unlink' -- 'params' is (classname, nodeid, propname)
-            'retire' -- 'params' is None
+            'retired' or 'restored' -- 'params' is None
 
             'creator' -- the user performing the action, which defaults to
             the current user.
         """
         if __debug__:
-            logging.getLogger('roundup.hyperdb').debug(
+            logging.getLogger('roundup.hyperdb.backend').debug(
                 'addjournal %s%s %s %r %s %r'%(classname,
                 nodeid, action, params, creator, creation))
         if creator is None:
@@ -583,7 +585,7 @@ def addjournal(self, classname, nodeid, action, params, creator=None,
     def setjournal(self, classname, nodeid, journal):
         """Set the journal to the "journal" list."""
         if __debug__:
-            logging.getLogger('roundup.hyperdb').debug(
+            logging.getLogger('roundup.hyperdb.backend').debug(
                 'setjournal %s%s %r'%(classname, nodeid, journal))
         self.transactions.append((self.doSetJournal, (classname, nodeid,
             journal)))
@@ -685,7 +687,7 @@ def pack(self, pack_before):
                         packed += 1
                 db[key] = marshal.dumps(l)
 
-                logging.getLogger('roundup.hyperdb').info(
+                logging.getLogger('roundup.hyperdb.backend').info(
                     'packed %d %s items'%(packed, classname))
 
             if db_type == 'gdbm':
@@ -708,7 +710,7 @@ def commit(self, fail_ok=False):
 
         The only backend this seems to affect is postgres.
         """
-        logging.getLogger('roundup.hyperdb').info('commit %s transactions'%(
+        logging.getLogger('roundup.hyperdb.backend').info('commit %s transactions'%(
             len(self.transactions)))
 
         # keep a handle to all the database files opened
@@ -833,7 +835,7 @@ def doDestroyNode(self, classname, nodeid):
     def rollback(self):
         """ Reverse all actions from the current transaction.
         """
-        logging.getLogger('roundup.hyperdb').info('rollback %s transactions'%(
+        logging.getLogger('roundup.hyperdb.backend').info('rollback %s transactions'%(
             len(self.transactions)))
 
         for method, args in self.transactions:
@@ -2098,7 +2100,8 @@ def export_journals(self):
         properties = self.getprops()
         r = []
         for nodeid in self.getnodeids():
-            for nodeid, date, user, action, params in self.history(nodeid):
+            for nodeid, date, user, action, params in self.history(nodeid,
+                            enforceperm=False, skipquiet=False):
                 date = date.get_tuple()
                 if action == 'set':
                     export_data = {}

roundup/backends/rdbms_common.py

@@ -533,7 +533,7 @@ def update_class(self, spec, old_spec, force=0):
         if not self.config.RDBMS_ALLOW_ALTER:
             raise DatabaseError(_('ALTER operation disallowed: %r -> %r.'%(old_spec, new_spec)))
 
-        logger = logging.getLogger('roundup.hyperdb')
+        logger = logging.getLogger('roundup.hyperdb.backend')
         logger.info('update_class %s'%spec.classname)
 
         logger.debug('old_spec %r'%(old_spec,))
@@ -858,7 +858,7 @@ def clear(self):
         Note: I don't commit here, which is different behaviour to the
               "nuke from orbit" behaviour in the dbs.
         """
-        logging.getLogger('roundup.hyperdb').info('clear')
+        logging.getLogger('roundup.hyperdb.backend').info('clear')
         for cn in self.classes:
             sql = 'delete from _%s'%cn
             self.sql(sql)
@@ -1194,7 +1194,7 @@ def destroynode(self, classname, nodeid):
         """Remove a node from the database. Called exclusively by the
            destroy() method on Class.
         """
-        logging.getLogger('roundup.hyperdb').info('destroynode %s%s'%(
+        logging.getLogger('roundup.hyperdb.backend').info('destroynode %s%s'%(
             classname, nodeid))
 
         # make sure the node exists
@@ -1263,9 +1263,14 @@ def addjournal(self, classname, nodeid, action, params, creator=None,
         """ Journal the Action
         'action' may be:
 
-            'create' or 'set' -- 'params' is a dictionary of property values
+            'set' -- 'params' is a dictionary of property values
+            'create' -- 'params' is an empty dictionary as of
+                      Wed Nov 06 11:38:43 2002 +0000
             'link' or 'unlink' -- 'params' is (classname, nodeid, propname)
-            'retire' -- 'params' is None
+            'retired' or 'restored' -- 'params' is None
+
+            'creator' -- the user performing the action, which defaults to
+            the current user.
         """
         # handle supply of the special journalling parameters (usually
         # supplied on importing an existing database)
@@ -1409,7 +1414,7 @@ def pack(self, pack_before):
     def sql_commit(self, fail_ok=False):
         """ Actually commit to the database.
         """
-        logging.getLogger('roundup.hyperdb').info('commit')
+        logging.getLogger('roundup.hyperdb.backend').info('commit')
 
         self.conn.commit()
 
@@ -1455,7 +1460,7 @@ def rollback(self):
         Undo all the changes made since the database was opened or the last
         commit() or rollback() was performed.
         """
-        logging.getLogger('roundup.hyperdb').info('rollback')
+        logging.getLogger('roundup.hyperdb.backend').info('rollback')
 
         self.sql_rollback()
 
@@ -1470,7 +1475,7 @@ def rollback(self):
         self.clearCache()
 
     def sql_close(self):
-        logging.getLogger('roundup.hyperdb').info('close')
+        logging.getLogger('roundup.hyperdb.backend').info('close')
         self.conn.close()
 
     def close(self):
@@ -2955,7 +2960,8 @@ def export_journals(self):
         properties = self.getprops()
         r = []
         for nodeid in self.getnodeids():
-            for nodeid, date, user, action, params in self.history(nodeid):
+            for nodeid, date, user, action, params in self.history(nodeid,
+                            enforceperm=False, skipquiet=False):
                 date = date.get_tuple()
                 if action == 'set':
                     export_data = {}

roundup/cgi/templating.py

@@ -973,7 +973,7 @@ def history(self, direction='descending', dre=re.compile('^\d+$'),
                         classname, id, current[prop_n])
 
         # get the journal, sort and reverse
-        history = self._klass.history(self._nodeid)
+        history = self._klass.history(self._nodeid, skipquiet=(not showall))
         history.sort()
         history.reverse()
 
@@ -987,22 +987,13 @@ def history(self, direction='descending', dre=re.compile('^\d+$'),
         for id, evt_date, user, action, args in history:
             date_s = str(evt_date.local(timezone)).replace("."," ")
             arg_s = ''
-            if action == 'link' and type(args) == type(()):
+            if action in ['link', 'unlink'] and type(args) == type(()):
                 if len(args) == 3:
                     linkcl, linkid, key = args
                     arg_s += '<a rel="nofollow" href="%s%s">%s%s %s</a>'%(linkcl, linkid,
                         linkcl, linkid, key)
                 else:
                     arg_s = str(args)
-
-            elif action == 'unlink' and type(args) == type(()):
-                if len(args) == 3:
-                    linkcl, linkid, key = args
-                    arg_s += '<a rel="nofollow" href="%s%s">%s%s %s</a>'%(linkcl, linkid,
-                        linkcl, linkid, key)
-                else:
-                    arg_s = str(args)
-
             elif type(args) == type({}):
                 cell = []
                 for k in args.keys():
@@ -1041,10 +1032,7 @@ def history(self, direction='descending', dre=re.compile('^\d+$'),
                         except NoTemplate:
                             hrefable = 0
 
-                    if (not showall) and prop.quiet:
-                        # Omit quiet properties from history/changelog
-                        continue
-                    elif isinstance(prop, hyperdb.Multilink) and args[k]:
+                    if isinstance(prop, hyperdb.Multilink) and args[k]:
                         ml = []
                         for linkid in args[k]:
                             if isinstance(linkid, type(())):