From 7ba405d381136599d321f3d47eec1b1f3737ba75 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 4 Feb 2024 16:51:45 +0100 Subject: [PATCH] [PostgreSQL provider] Add missing namespace escaping in SQL requests --- src/providers/postgres/qgspostgresconn.cpp | 35 ++++++++++--------- .../qgspostgresdataitemguiprovider.cpp | 2 +- .../qgspostgresproviderconnection.cpp | 5 ++- .../test_processing_importintopostgis.py | 6 ++-- .../python/test_qgsdatabaseschemacombobox.py | 12 +++---- .../src/python/test_qgsdatabaseschemamodel.py | 20 +++++------ tests/testdata/provider/testdata_pg.sql | 12 +++---- 7 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/providers/postgres/qgspostgresconn.cpp b/src/providers/postgres/qgspostgresconn.cpp index c940ed8e6c99..73ecd1ab7b95 100644 --- a/src/providers/postgres/qgspostgresconn.cpp +++ b/src/providers/postgres/qgspostgresconn.cpp @@ -48,6 +48,16 @@ const int PG_DEFAULT_TIMEOUT = 30; +static QString quotedString( const QString &v ) +{ + QString result = v; + result.replace( '\'', QLatin1String( "''" ) ); + if ( result.contains( '\\' ) ) + return result.replace( '\\', QLatin1String( "\\\\" ) ).prepend( "E'" ).append( '\'' ); + else + return result.prepend( '\'' ).append( '\'' ); +} + QgsPostgresResult::~QgsPostgresResult() { if ( mRes ) @@ -707,10 +717,10 @@ bool QgsPostgresConn::getTableInfo( bool searchGeometryColumnsOnly, bool searchP sql += QLatin1String( " AND n.nspname='public'" ); if ( !schema.isEmpty() ) - sql += QStringLiteral( " AND %1='%2'" ).arg( schemaName, schema ); + sql += QStringLiteral( " AND %1=%2" ).arg( schemaName, quotedString( schema ) ); if ( !name.isEmpty() ) - sql += QStringLiteral( " AND %1='%2'" ).arg( tableName, name ); + sql += QStringLiteral( " AND %1=%2" ).arg( tableName, quotedString( name ) ); sql += QString( " GROUP BY 1,2,3,4,5,6,7,c.oid,11" ); @@ -850,10 +860,10 @@ bool QgsPostgresConn::getTableInfo( bool searchGeometryColumnsOnly, bool searchP sql += QLatin1String( " AND n.nspname='public'" ); if ( !schema.isEmpty() ) - sql += QStringLiteral( " AND n.nspname='%1'" ).arg( schema ); + sql += QStringLiteral( " AND n.nspname=%1" ).arg( quotedString( schema ) ); if ( !name.isEmpty() ) - sql += QStringLiteral( " AND c.relname ='%1'" ).arg( name ); + sql += QStringLiteral( " AND c.relname=%1" ).arg( quotedString( name ) ); // skip columns of which we already derived information from the metadata tables if ( nColumns > 0 ) @@ -992,10 +1002,11 @@ bool QgsPostgresConn::getTableInfo( bool searchGeometryColumnsOnly, bool searchP sql += QLatin1String( " AND pg_namespace.nspname='public'" ); if ( !schema.isEmpty() ) - sql += QStringLiteral( " AND pg_namespace.nspname='%1'" ).arg( schema ); + sql += QStringLiteral( " AND pg_namespace.nspname=%1" ).arg( quotedString( schema ) ); if ( !name.isEmpty() ) - sql += QStringLiteral( " AND pg_class.relname='%1'" ).arg( name ); + sql += QStringLiteral( " AND pg_class.relname=%1" ).arg( quotedString( name ) ); + sql += QLatin1String( " GROUP BY 1,2,3,4" ); @@ -1339,16 +1350,6 @@ QString QgsPostgresConn::quotedIdentifier( const QString &ident ) return result.prepend( '\"' ).append( '\"' ); } -static QString quotedString( const QString &v ) -{ - QString result = v; - result.replace( '\'', QLatin1String( "''" ) ); - if ( result.contains( '\\' ) ) - return result.replace( '\\', QLatin1String( "\\\\" ) ).prepend( "E'" ).append( '\'' ); - else - return result.prepend( '\'' ).append( '\'' ); -} - static QString doubleQuotedMapValue( const QString &v ) { QString result = v; @@ -2963,7 +2964,7 @@ int QgsPostgresConn::crsToSrid( const QgsCoordinateReferenceSystem &crs ) return -1; const QString authName = authParts.first(); const QString authId = authParts.last(); - QgsPostgresResult result( PQexec( QStringLiteral( "SELECT srid FROM spatial_ref_sys WHERE auth_name='%1' AND auth_srid=%2" ).arg( authName, authId ) ) ); + QgsPostgresResult result( PQexec( QStringLiteral( "SELECT srid FROM spatial_ref_sys WHERE auth_name=%1 AND auth_srid=%2" ).arg( quotedString( authName ), authId ) ) ); if ( result.PQresultStatus() == PGRES_TUPLES_OK ) { diff --git a/src/providers/postgres/qgspostgresdataitemguiprovider.cpp b/src/providers/postgres/qgspostgresdataitemguiprovider.cpp index bd059dbd5181..2c4306e7118a 100644 --- a/src/providers/postgres/qgspostgresdataitemguiprovider.cpp +++ b/src/providers/postgres/qgspostgresdataitemguiprovider.cpp @@ -307,7 +307,7 @@ void QgsPostgresDataItemGuiProvider::deleteSchema( QgsPGSchemaItem *schemaItem, return; } - const QString sql = QStringLiteral( "SELECT table_name FROM information_schema.tables WHERE table_schema='%1'" ).arg( schemaItem->name() ); + const QString sql = QStringLiteral( "SELECT table_name FROM information_schema.tables WHERE table_schema=%1" ).arg( QgsPostgresConn::quotedValue( schemaItem->name() ) ); QgsPostgresResult result( conn->LoggedPQexec( "QgsPostgresDataItemGuiProvider", sql ) ); if ( result.PQresultStatus() != PGRES_TUPLES_OK ) { diff --git a/src/providers/postgres/qgspostgresproviderconnection.cpp b/src/providers/postgres/qgspostgresproviderconnection.cpp index 7d3c522e84ad..1b0fa8e2a720 100644 --- a/src/providers/postgres/qgspostgresproviderconnection.cpp +++ b/src/providers/postgres/qgspostgresproviderconnection.cpp @@ -340,12 +340,11 @@ QList QgsPostgresProviderC { const QList pks = executeSqlPrivate( QStringLiteral( R"( WITH pkrelid AS ( - SELECT indexrelid AS idxri FROM pg_index WHERE indrelid='%1.%2'::regclass AND (indisprimary OR indisunique) + SELECT indexrelid AS idxri FROM pg_index WHERE indrelid=%1::regclass AND (indisprimary OR indisunique) ORDER BY CASE WHEN indisprimary THEN 1 ELSE 2 END LIMIT 1) SELECT attname FROM pg_index,pg_attribute, pkrelid WHERE indexrelid=pkrelid.idxri AND indrelid=attrelid AND pg_attribute.attnum=any(pg_index.indkey); - )" ).arg( QgsPostgresConn::quotedIdentifier( pr.schemaName ), - QgsPostgresConn::quotedIdentifier( pr.tableName ) ), false ); + )" ).arg( QgsPostgresConn::quotedValue( QString( QgsPostgresConn::quotedIdentifier( pr.schemaName ) + "." + QgsPostgresConn::quotedIdentifier( pr.tableName ) ) ) ), false ); QStringList pkNames; for ( const QVariantList &pk : std::as_const( pks ) ) { diff --git a/tests/src/python/test_processing_importintopostgis.py b/tests/src/python/test_processing_importintopostgis.py index 93a6e834110f..8f387c39aeb1 100644 --- a/tests/src/python/test_processing_importintopostgis.py +++ b/tests/src/python/test_processing_importintopostgis.py @@ -58,7 +58,7 @@ def setUpClass(cls): settings.setValue('database', 'qgis_test') def test_import(self): - """Test algorithm with CamelCaseSchema""" + """Test algorithm with CamelCase'singlequote'Schema""" alg = self.registry.createAlgorithmById("qgis:importintopostgis") self.assertIsNotNone(alg) @@ -76,7 +76,7 @@ def test_import(self): 'LOWERCASE_NAMES': True, 'OVERWRITE': True, 'PRIMARY_KEY': None, - 'SCHEMA': 'CamelCaseSchema', + 'SCHEMA': "CamelCase'singlequote'Schema", 'TABLENAME': table_name } @@ -90,7 +90,7 @@ def test_import(self): # Check that data have been imported correctly exported = QgsVectorLayer(unitTestDataPath() + '/points.shp', 'exported') self.assertTrue(exported.isValid()) - imported = QgsVectorLayer(f"service='qgis_test' table=\"CamelCaseSchema\".\"{table_name}\" (geom)", 'imported', 'postgres') + imported = QgsVectorLayer(f"service='qgis_test' table=\"CamelCase'singlequote'Schema\".\"{table_name}\" (geom)", 'imported', 'postgres') self.assertTrue(imported.isValid()) imported_fields = [f.name() for f in imported.fields()] for f in exported.fields(): diff --git a/tests/src/python/test_qgsdatabaseschemacombobox.py b/tests/src/python/test_qgsdatabaseschemacombobox.py index 645cc7792ca4..3e150677efbf 100644 --- a/tests/src/python/test_qgsdatabaseschemacombobox.py +++ b/tests/src/python/test_qgsdatabaseschemacombobox.py @@ -50,10 +50,10 @@ def testCombo(self): self.assertGreaterEqual(m.comboBox().count(), 3) text = [m.comboBox().itemText(i) for i in range(m.comboBox().count())] - self.assertIn('CamelCaseSchema', text) + self.assertIn("CamelCase'singlequote'Schema", text) self.assertIn('qgis_test', text) - self.assertLess(text.index('CamelCaseSchema'), text.index('qgis_test')) - self.assertEqual(m.currentSchema(), 'CamelCaseSchema') + self.assertLess(text.index("CamelCase'singlequote'Schema"), text.index('qgis_test')) + self.assertEqual(m.currentSchema(), "CamelCase'singlequote'Schema") m.setSchema('qgis_test') self.assertEqual(m.currentSchema(), 'qgis_test') @@ -124,10 +124,10 @@ def testComboWithEmpty(self): text = [m.comboBox().itemText(i) for i in range(m.comboBox().count())] self.assertFalse(text[0]) - self.assertIn('CamelCaseSchema', text) + self.assertIn("CamelCase'singlequote'Schema", text) self.assertIn('qgis_test', text) - self.assertLess(text.index('CamelCaseSchema'), text.index('qgis_test')) - self.assertEqual(m.currentSchema(), 'CamelCaseSchema') + self.assertLess(text.index("CamelCase'singlequote'Schema"), text.index('qgis_test')) + self.assertEqual(m.currentSchema(), "CamelCase'singlequote'Schema") m.setSchema('qgis_test') self.assertEqual(m.currentSchema(), 'qgis_test') diff --git a/tests/src/python/test_qgsdatabaseschemamodel.py b/tests/src/python/test_qgsdatabaseschemamodel.py index 1e84287043dd..8f09a4d1cc78 100644 --- a/tests/src/python/test_qgsdatabaseschemamodel.py +++ b/tests/src/python/test_qgsdatabaseschemamodel.py @@ -48,7 +48,7 @@ def testModel(self): self.assertEqual(model.columnCount(), 1) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertEqual(model.data(model.index(schemas.index('qgis_test'), 0, QModelIndex()), Qt.ItemDataRole.ToolTipRole), 'qgis_test') self.assertIsNone(model.data(model.index(model.rowCount(), 0, QModelIndex()), Qt.ItemDataRole.DisplayRole)) @@ -62,7 +62,7 @@ def testModel(self): self.assertEqual(model.rowCount(), old_count + 1) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertIn('myNewSchema', schemas) @@ -72,7 +72,7 @@ def testModel(self): self.assertEqual(model.rowCount(), old_count + 3) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertIn('myNewSchema', schemas) self.assertIn('myNewSchema2', schemas) @@ -85,7 +85,7 @@ def testModel(self): self.assertEqual(model.rowCount(), old_count + 2) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertNotIn('myNewSchema', schemas) self.assertNotIn('myNewSchema2', schemas) @@ -98,7 +98,7 @@ def testModel(self): self.assertEqual(model.rowCount(), old_count) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertNotIn('myNewSchema3', schemas) self.assertNotIn('myNewSchema4', schemas) @@ -115,7 +115,7 @@ def test_model_allow_empty(self): schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertFalse(model.data(model.index(0, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole)) self.assertTrue(model.data(model.index(0, 0, QModelIndex()), QgsDatabaseSchemaModel.Role.RoleEmpty)) @@ -131,7 +131,7 @@ def test_model_allow_empty(self): self.assertEqual(model.rowCount(), old_count + 2) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertIn('myNewSchema', schemas) self.assertFalse(model.data(model.index(0, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole)) @@ -154,7 +154,7 @@ def test_model_allow_empty(self): self.assertEqual(model.rowCount(), old_count + 4) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertIn('myNewSchema', schemas) self.assertIn('myNewSchema2', schemas) @@ -170,7 +170,7 @@ def test_model_allow_empty(self): self.assertEqual(model.rowCount(), old_count + 3) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertNotIn('myNewSchema', schemas) self.assertNotIn('myNewSchema2', schemas) @@ -186,7 +186,7 @@ def test_model_allow_empty(self): self.assertEqual(model.rowCount(), old_count + 1) schemas = [model.data(model.index(r, 0, QModelIndex()), Qt.ItemDataRole.DisplayRole) for r in range(model.rowCount())] self.assertIn('public', schemas) - self.assertIn('CamelCaseSchema', schemas) + self.assertIn("CamelCase'singlequote'Schema", schemas) self.assertIn('qgis_test', schemas) self.assertNotIn('myNewSchema3', schemas) self.assertNotIn('myNewSchema4', schemas) diff --git a/tests/testdata/provider/testdata_pg.sql b/tests/testdata/provider/testdata_pg.sql index 6de7bc388211..06bf9ee4fc82 100644 --- a/tests/testdata/provider/testdata_pg.sql +++ b/tests/testdata/provider/testdata_pg.sql @@ -31,12 +31,12 @@ ALTER DEFAULT PRIVILEGES IN SCHEMA qgis_test GRANT ALL ON TABLES TO public; ALTER DEFAULT PRIVILEGES IN SCHEMA qgis_test GRANT ALL ON SEQUENCES TO public; ---- Create "CamelCaseSchema" schema -DROP SCHEMA IF EXISTS "CamelCaseSchema" CASCADE; -CREATE SCHEMA "CamelCaseSchema"; -GRANT ALL ON SCHEMA "CamelCaseSchema" TO public; -ALTER DEFAULT PRIVILEGES IN SCHEMA "CamelCaseSchema" GRANT ALL ON TABLES TO public; -ALTER DEFAULT PRIVILEGES IN SCHEMA "CamelCaseSchema" GRANT ALL ON SEQUENCES TO public; +--- Create "CamelCase'singlequote'Schema" schema +DROP SCHEMA IF EXISTS "CamelCase'singlequote'Schema" CASCADE; +CREATE SCHEMA "CamelCase'singlequote'Schema"; +GRANT ALL ON SCHEMA "CamelCase'singlequote'Schema" TO public; +ALTER DEFAULT PRIVILEGES IN SCHEMA "CamelCase'singlequote'Schema" GRANT ALL ON TABLES TO public; +ALTER DEFAULT PRIVILEGES IN SCHEMA "CamelCase'singlequote'Schema" GRANT ALL ON SEQUENCES TO public; SET default_tablespace = '';