jenkins-bot has submitted this change and it was merged.
Change subject: Abstract out DbStorage superclass from BasicDbStorage and
RevisionStorage.
......................................................................
Abstract out DbStorage superclass from BasicDbStorage and RevisionStorage.
Also make sure that RevisionStorage has the security improvements from 64bf13a
Change-Id: Ia00a6f96f3ac23085f43b5f6bfc8bae88b8e21b0
---
M Flow.php
M includes/Data/BoardHistoryStorage.php
M includes/Data/ObjectManager.php
M includes/Data/RevisionStorage.php
4 files changed, 88 insertions(+), 72 deletions(-)
Approvals:
EBernhardson: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Flow.php b/Flow.php
index 20d349c..a39f6e7 100755
--- a/Flow.php
+++ b/Flow.php
@@ -104,6 +104,7 @@
$wgAutoloadClasses['Flow\Data\BoardHistoryStorage'] = $dir .
'includes/Data/BoardHistoryStorage.php';
$wgAutoloadClasses['Flow\Data\BoardHistoryIndex'] = $dir .
'includes/Data/BoardHistoryStorage.php';
$wgAutoloadClasses['Flow\Data\ObjectStorage'] = $dir .
'includes/Data/ObjectManager.php';
+$wgAutoloadClasses['Flow\Data\DbStorage'] = $dir .
'includes/Data/ObjectManager.php';
$wgAutoloadClasses['Flow\Data\BasicDbStorage'] = $dir .
'includes/Data/ObjectManager.php';
$wgAutoloadClasses['Flow\Data\ObjectMapper'] = $dir .
'includes/Data/ObjectManager.php';
$wgAutoloadClasses['Flow\Data\BasicObjectMapper'] = $dir .
'includes/Data/ObjectManager.php';
@@ -119,6 +120,7 @@
$wgAutoloadClasses['Flow\Data\PostRevisionRecentChanges'] = $dir .
'includes/Data/RecentChanges.php';
$wgAutoloadClasses['Flow\Data\HeaderRecentChanges'] = $dir .
'includes/Data/RecentChanges.php';
$wgAutoloadClasses['Flow\Data\Merger'] = $dir .
'includes/Data/RevisionStorage.php';
+$wgAutoloadClasses['Flow\Data\RawSql'] = $dir .
'includes/Data/ObjectManager.php';
$wgAutoloadClasses['Flow\RecentChanges\Formatter'] = $dir .
'includes/RecentChanges/Formatter.php';
$wgAutoloadClasses['Flow\Log\Logger'] = $dir . 'includes/Log/Logger.php';
$wgAutoloadClasses['Flow\Log\Formatter'] = $dir . 'includes/Log/Formatter.php';
diff --git a/includes/Data/BoardHistoryStorage.php
b/includes/Data/BoardHistoryStorage.php
index 4e91818..e05f967 100644
--- a/includes/Data/BoardHistoryStorage.php
+++ b/includes/Data/BoardHistoryStorage.php
@@ -8,13 +8,9 @@
use Flow\DbFactory;
use Flow\Container;
-class BoardHistoryStorage implements WritableObjectStorage {
+class BoardHistoryStorage extends DbStorage {
protected $dbFactory;
-
- public function __construct( DbFactory $dbFactory ) {
- $this->dbFactory = $dbFactory;
- }
function find( array $attributes, array $options = array() ) {
$multi = $this->findMulti( $attributes, $options );
@@ -37,7 +33,7 @@
}
function findHeaderHistory( array $queries, array $options = array() ) {
- $queries = reset( $queries );
+ $queries = $this->preprocessSqlArray( reset( $queries ) );
$res = $this->dbFactory->getDB( DB_SLAVE )->select(
array( 'flow_header_revision', 'flow_revision' ),
@@ -58,10 +54,12 @@
}
function findTopicListHistory( array $queries, array $options = array()
) {
+ $queries = $this->preprocessSqlArray( reset( $queries ) );
+
$res = $this->dbFactory->getDB( DB_SLAVE )->select(
array( 'flow_topic_list', 'flow_tree_revision',
'flow_revision' ),
array( '*' ),
- array( 'tree_rev_id = rev_id', 'tree_rev_descendant_id
= topic_id' ) + UUID::convertUUIDs( reset( $queries ) ),
+ array( 'tree_rev_id = rev_id', 'tree_rev_descendant_id
= topic_id' ) + $queries,
__METHOD__,
$options
);
diff --git a/includes/Data/ObjectManager.php b/includes/Data/ObjectManager.php
index d42360d..a8794a1 100644
--- a/includes/Data/ObjectManager.php
+++ b/includes/Data/ObjectManager.php
@@ -602,18 +602,89 @@
}
/**
+ * Base class for all WritableObjectStorage implementers
+ * which use a database as the backing store.
+ *
+ * Includes some utility methods for database management and
+ * SQL security.
+ */
+abstract class DbStorage implements WritableObjectStorage {
+ protected $dbFactory;
+
+ public function __construct( DbFactory $dbFactory ) {
+ $this->dbFactory = $dbFactory;
+ }
+
+ /**
+ * At the moment, does three things:
+ * 1. Finds UUID objects and returns their database representation.
+ * 2. Checks for unarmoured raw SQL and errors out if it exists.
+ * 3. Finds armoured raw SQL and expands it out.
+ *
+ * @param array $data Query conditions for DatabaseBase::select
+ * @return array query conditions escaped for use
+ */
+ protected function preprocessSqlArray( array $data ) {
+ // Assuming that all databases have the same escaping settings.
+ $db = $this->dbFactory->getDB( DB_SLAVE );
+
+ $data = UUID::convertUUIDs( $data );
+
+ foreach( $data as $key => $value ) {
+ if ( $value instanceof RawSql ) {
+ $data[$key] = $value->getSql( $db );
+ } elseif ( is_numeric( $key ) ) {
+ throw new \MWException( "Unescaped raw SQL
found in " . __METHOD__ );
+ } elseif ( ! preg_match( '/^[A-Za-z0-9\._]+$/', $key )
) {
+ throw new \MWException( "Dangerous SQL field
name '$key' found in " . __METHOD__ );
+ }
+ }
+
+ return $data;
+ }
+
+ /**
+ * Internal security function which checks a row object
+ * (for inclusion as a condition or a row for insert/update)
+ * for any numeric keys (= raw SQL), or field names with
+ * potentially unsafe characters.
+ *
+ * @param array $row The row to check.
+ * @return boolean True if raw SQL is found
+ */
+ protected function hasUnescapedSQL( array $row ) {
+ foreach( $row as $key => $value ) {
+ if ( $value instanceof RawSql ) {
+ // Specifically allowed SQL
+ continue;
+ }
+
+ if ( is_numeric( $key ) ) {
+ return true;
+ }
+
+ if ( ! preg_match( '/^[A-Za-z0-9\._]+$/', $key ) ) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+}
+
+/**
* Standard backing store for data model with no special cases which is stored
* in a single table in mysql.
*
* Doesn't support updating primary key value yet
* Doesn't support auto-increment pk yet
*/
-class BasicDbStorage implements WritableObjectStorage {
+class BasicDbStorage extends DbStorage {
public function __construct( DbFactory $dbFactory, $table, array
$primaryKey ) {
if ( !$primaryKey ) {
throw new \MWException( 'PK required' );
}
- $this->dbFactory = $dbFactory;
+ parent::__construct( $dbFactory );
$this->table = $table;
$this->primaryKey = $primaryKey;
}
@@ -739,60 +810,6 @@
public function getPrimaryKeyColumns() {
return $this->primaryKey;
- }
-
- /**
- * At the moment, does three things:
- * 1. Finds UUID objects and returns their database representation.
- * 2. Checks for unarmoured raw SQL and errors out if it exists.
- * 3. Finds armoured raw SQL and expands it out.
- * @param [type] $data [description]
- * @return [type] [description]
- */
- protected function preprocessSqlArray( $data ) {
- // Assuming that all databases have the same escaping settings.
- $db = $this->dbFactory->getDB( DB_SLAVE );
-
- $data = UUID::convertUUIDs( $data );
-
- foreach( $data as $key => $value ) {
- if ( $value instanceof RawSql ) {
- $data[$key] = $value->getSql( $db );
- } elseif ( is_numeric( $key ) ) {
- throw new \MWException( "Unescaped raw SQL
found in " . __METHOD__ );
- } elseif ( ! preg_match( '/^[A-Za-z0-9\._]+$/', $key )
) {
- throw new \MWException( "Dangerous SQL field
name '$key' found in " . __METHOD__ );
- }
- }
-
- return $data;
- }
-
- /**
- * Internal security function which checks a row object
- * (for inclusion as a condition or a row for insert/update)
- * for any numeric keys (= raw SQL), or field names with
- * potentially unsafe characters.
- * @param array $row The row to check.
- * @return boolean True if raw SQL is found
- */
- protected function hasUnescapedSQL( array $row ) {
- foreach( $row as $key => $value ) {
- if ( $value instanceof RawSql ) {
- // Specifically allowed SQL
- continue;
- }
-
- if ( is_numeric( $key ) ) {
- return true;
- }
-
- if ( ! preg_match( '/^[A-Za-z0-9\._]+$/', $key ) ) {
- return true;
- }
- }
-
- return false;
}
}
diff --git a/includes/Data/RevisionStorage.php
b/includes/Data/RevisionStorage.php
index d9bd218..f85db8f 100644
--- a/includes/Data/RevisionStorage.php
+++ b/includes/Data/RevisionStorage.php
@@ -9,7 +9,7 @@
use ExternalStore;
use User;
-abstract class RevisionStorage implements WritableObjectStorage {
+abstract class RevisionStorage extends DbStorage {
static protected $allowedUpdateColumns = array(
'rev_mod_state',
'rev_mod_user_id',
@@ -17,7 +17,6 @@
'rev_mod_timestamp',
'rev_mod_reason',
);
- protected $dbFactory;
protected $externalStores;
abstract protected function joinTable();
@@ -29,7 +28,7 @@
abstract protected function removeRelated( array $row );
public function __construct( DbFactory $dbFactory, $externalStore ) {
- $this->dbFactory = $dbFactory;
+ parent::__construct( $dbFactory );
$this->externalStore = $externalStore;
}
@@ -48,7 +47,7 @@
$res = $dbr->select(
array( $this->joinTable(), 'rev' => 'flow_revision' ),
'*',
- UUID::convertUUIDs( $attributes ),
+ $this->preprocessSqlArray( $attributes ),
__METHOD__,
$options,
array( 'rev' => array( 'JOIN', $this->joinField() . ' =
rev_id' ) )
@@ -133,7 +132,7 @@
$res = $dbr->select(
$this->joinTable(),
array( $joinField => "MAX( {$this->joinField()} )" ),
- $this->buildCompositeInCondition( $dbr,
$duplicator->getUniqueQueries() ),
+ $this->preprocessSqlArray(
$this->buildCompositeInCondition( $dbr, $duplicator->getUniqueQueries() ) ),
__METHOD__,
array( 'GROUP BY' => $keys )
);
@@ -244,7 +243,7 @@
$dbw = $this->dbFactory->getDB( DB_MASTER );
$res = $dbw->insert(
'flow_revision',
- $rev,
+ $this->preprocessSqlArray( $rev ),
__METHOD__
);
if ( !$res ) {
@@ -286,7 +285,7 @@
$dbw = $this->dbFactory->getDB( DB_MASTER );
$res = $dbw->update(
'flow_revision',
- $rev,
+ $this->preprocessSqlArray( $rev ),
array( 'rev_id' => $old['rev_id'] ),
__METHOD__
);
@@ -367,7 +366,7 @@
$dbw = $this->dbFactory->getDB( DB_MASTER );
$res = $dbw->insert(
$this->joinTable(),
- UUID::convertUUIDs( $tree ),
+ $this->preprocessSqlArray( $tree ),
__METHOD__
);
@@ -424,7 +423,7 @@
protected function insertRelated( array $row, array $header ) {
$res = $this->dbFactory->getDB( DB_MASTER )->insert(
$this->joinTable(),
- $header,
+ $this->preprocessSqlArray( $header ),
__METHOD__
);
if ( !$res ) {
--
To view, visit https://gerrit.wikimedia.org/r/99035
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia00a6f96f3ac23085f43b5f6bfc8bae88b8e21b0
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Werdna <[email protected]>
Gerrit-Reviewer: EBernhardson <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
