From 8974d199958457566cb431d2eed546752517475e Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Sun, 17 Apr 2016 15:41:28 -0400 Subject: [PATCH] Modify documentation of select() method for Database. * Add a note in Database.php that docs are in IDatabase.php, maybe its just me, but I didn't realize that was where the docs are (even though in retrospect that should have been obvious) and wasted a lot of time because I didn't realize that. * Change references to IDatabase::tableName() in doc to DatabaseBase::tableName(), since tableName is not in the IDatabase interface. * Be explicit in docs for select() about which parts are safe for user input. I think its important to be very explicit about this. No code changes, just doc changes. Change-Id: I3a66477afc6a5f54855062482ef04c2966e468f6 --- includes/db/Database.php | 5 +++++ includes/db/IDatabase.php | 20 ++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/includes/db/Database.php b/includes/db/Database.php index c065ee952a..c36cfdb453 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -1226,6 +1226,7 @@ abstract class DatabaseBase implements IDatabase { return ''; } + // See IDatabase::select for the docs for this function public function select( $table, $vars, $conds = '', $fname = __METHOD__, $options = [], $join_conds = [] ) { $sql = $this->selectSQLText( $table, $vars, $conds, $fname, $options, $join_conds ); @@ -1668,6 +1669,8 @@ abstract class DatabaseBase implements IDatabase { * themselves. Pass the canonical name to such functions. This is only needed * when calling query() directly. * + * @note This function does not sanitize user input. It is not safe to use + * this function to escape user input. * @param string $name Database table name * @param string $format One of: * quoted - Automatically pass the table name through addIdentifierQuotes() @@ -1981,6 +1984,8 @@ abstract class DatabaseBase implements IDatabase { * Returns if the given identifier looks quoted or not according to * the database convention for quoting identifiers . * + * @note Do not use this to determine if untrusted input is safe. + * A malicious user can trick this function. * @param string $name * @return bool */ diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php index 8b1c3dff22..710efb2ca6 100644 --- a/includes/db/IDatabase.php +++ b/includes/db/IDatabase.php @@ -520,9 +520,11 @@ interface IDatabase { * for use in field names (e.g. a.user_name). * * All of the table names given here are automatically run through - * IDatabase::tableName(), which causes the table prefix (if any) to be + * DatabaseBase::tableName(), which causes the table prefix (if any) to be * added, and various other table name mappings to be performed. * + * Do not use untrusted user input as a table name. Alias names should + * not have characters outside of the Basic multilingual plane. * * @param string|array $vars * @@ -537,6 +539,7 @@ interface IDatabase { * If an expression is given, care must be taken to ensure that it is * DBMS-independent. * + * Untrusted user input must not be passed to this parameter. * * @param string|array $conds * @@ -563,6 +566,10 @@ interface IDatabase { * - IDatabase::buildLike() * - IDatabase::conditional() * + * Untrusted user input is safe in the values of string keys, however untrusted + * input must not be used in the array key names or in the values of numeric keys. + * Escaping of untrusted input used in values of numeric keys should be done via + * IDatabase::addQuotes() * * @param string|array $options * @@ -628,8 +635,9 @@ interface IDatabase { * * The key of the array contains the table name or alias. The value is an * array with two elements, numbered 0 and 1. The first gives the type of - * join, the second is an SQL fragment giving the join condition for that - * table. For example: + * join, the second is the same as the $conds parameter. Thus it can be + * an SQL fragment, or an array where the string keys are equality and the + * numeric keys are SQL fragments all AND'd together. For example: * * array( 'page' => array( 'LEFT JOIN', 'page_latest=rev_id' ) ) * @@ -794,7 +802,7 @@ interface IDatabase { * IDatabase::affectedRows(). * * @param string $table Table name. This will be passed through - * IDatabase::tableName(). + * DatabaseBase::tableName(). * @param array $a Array of rows to insert * @param string $fname Calling function name (use __METHOD__) for logs/profiling * @param array $options Array of options @@ -807,7 +815,7 @@ interface IDatabase { * UPDATE wrapper. Takes a condition array and a SET array. * * @param string $table Name of the table to UPDATE. This will be passed through - * IDatabase::tableName(). + * DatabaseBase::tableName(). * @param array $values An array of values to SET. For each array element, * the key gives the field name, and the value gives the data to set * that field to. The data will be quoted by IDatabase::addQuotes(). @@ -1020,7 +1028,7 @@ interface IDatabase { * * @since 1.22 * - * @param string $table Table name. This will be passed through IDatabase::tableName(). + * @param string $table Table name. This will be passed through DatabaseBase::tableName(). * @param array $rows A single row or list of rows to insert * @param array $uniqueIndexes List of single field names or field name tuples * @param array $set An array of values to SET. For each array element, the -- 2.20.1