From d374a5f77e6b58e36f0e0e4419be18b95edcd7ff Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Mon, 21 Nov 2022 18:46:32 +0100 Subject: [PATCH 1/5] FOLIO-3645: schema name SQL injection SELECT EXISTS now uses a prepared statement. PostgreSQL doesn't support a prepared statement for CREATE SCHEMA and DROP SCHEMA. Instead we validate the schema name before use to mitigate any SQL injection attack. --- .../tenant/hibernate/HibernateSchemaService.java | 10 +++++----- .../org/folio/spring/tenant/service/SchemaService.java | 9 ++++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java b/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java index dbf4af3..ad52309 100644 --- a/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java +++ b/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java @@ -6,6 +6,7 @@ import java.nio.charset.StandardCharsets; import java.sql.Connection; import java.sql.DriverManager; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; @@ -41,8 +42,6 @@ import org.springframework.core.type.filter.AnnotationTypeFilter; import org.springframework.stereotype.Service; -import static org.hibernate.cfg.AvailableSettings.*; - @Service("hibernateSchemaService") public class HibernateSchemaService implements InitializingBean { @@ -177,9 +176,10 @@ private void dropSchema(Connection connection, String schema) throws SQLExceptio } private boolean schemaExists(Connection connection, String schema) throws SQLException { - try (Statement statement = connection.createStatement()) { - String queryTemplate = "SELECT EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = '%s');"; - ResultSet resultSet = statement.executeQuery(String.format(queryTemplate, schema)); + String sql = "SELECT EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = ?)"; + try (PreparedStatement statement = connection.prepareStatement(sql)) { + statement.setString(1, schema); + ResultSet resultSet = statement.executeQuery(); if (resultSet.next()) { return resultSet.getBoolean(1); } diff --git a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java index 32eee78..682672f 100644 --- a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java +++ b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java @@ -1,5 +1,6 @@ package org.folio.spring.tenant.service; +import java.util.regex.Pattern; import org.folio.spring.tenant.properties.BuildInfoProperties; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; @@ -7,11 +8,17 @@ @Service public class SchemaService { + private static final Pattern SCHEMA_REGEXP = Pattern.compile("[a-zA-Z0-9_]+"); + @Autowired private BuildInfoProperties buildInfoProperties; public String getSchema(String tenant) { - return String.format("%s_%s", tenant, buildInfoProperties.getArtifact()).replace("-", "_"); + var schema = String.format("%s_%s", tenant, buildInfoProperties.getArtifact()).replace("-", "_"); + if (! SCHEMA_REGEXP.matcher(schema).matches()) { + throw new IllegalArgumentException("Illegal character in schema name: " + schema); + } + return schema; } } From 701330dbcec2bfa771a827f37978767a1df67bb1 Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Tue, 22 Nov 2022 23:08:12 +0100 Subject: [PATCH 2/5] SchemaService test coverage --- pom.xml | 6 ++++ .../tenant/service/SchemaServiceTest.java | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java diff --git a/pom.xml b/pom.xml index 0223b41..5498dad 100644 --- a/pom.xml +++ b/pom.xml @@ -139,6 +139,12 @@ + + org.apache.maven.plugins + maven-surefire-plugin + 3.0.0-M7 + + diff --git a/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java b/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java new file mode 100644 index 0000000..c423831 --- /dev/null +++ b/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java @@ -0,0 +1,34 @@ +package org.folio.spring.tenant.service; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.folio.spring.tenant.properties.BuildInfoProperties; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.TestPropertySource; + +@SpringBootTest(classes = SchemaService.class) +@EnableConfigurationProperties(value = BuildInfoProperties.class) +@TestPropertySource(properties = "info.build.artifact=mod-foo") +class SchemaServiceTest { + + @Autowired + private SchemaService schemaService; + + @Test + void validSchema() { + assertThat(schemaService.getSchema("tenant123"), is("tenant123_mod_foo")); + } + + @ParameterizedTest + @ValueSource(strings = { "รค", "?", "abc+xyz", "a1." }) + void invalidSchema(String tenant) { + assertThrows(IllegalArgumentException.class, () -> schemaService.getSchema(tenant)); + } +} From 3200f683ebc281eed032117a0cf09be7a93f6b13 Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Tue, 22 Nov 2022 23:08:52 +0100 Subject: [PATCH 3/5] Fix regexp code smell: use \w, not [a-zA-Z0-9_] --- .../java/org/folio/spring/tenant/service/SchemaService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java index 682672f..0d72546 100644 --- a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java +++ b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java @@ -8,7 +8,8 @@ @Service public class SchemaService { - private static final Pattern SCHEMA_REGEXP = Pattern.compile("[a-zA-Z0-9_]+"); + // allowing only a-z A-Z 0-9 and _ prevents SQL injection + private static final Pattern SCHEMA_REGEXP = Pattern.compile("\\w+"); @Autowired private BuildInfoProperties buildInfoProperties; From a235a270a9a786d5312a585741dfe3f4a6aeebc7 Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Wed, 23 Nov 2022 10:23:00 +0100 Subject: [PATCH 4/5] Add TenantControllerTest, fix schema case, fix schemaExists arg --- pom.xml | 8 +++ tenant/pom.xml | 6 +++ .../hibernate/HibernateSchemaService.java | 12 ++--- .../spring/tenant/service/SchemaService.java | 8 +-- .../folio/spring/tenant/TestApplication.java | 20 +++++++ .../controller/TenantControllerTest.java | 31 +++++++++++ .../tenant/service/SchemaServiceTest.java | 4 +- tenant/src/test/resources/application.yml | 53 +++++++++++++++++++ 8 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 tenant/src/test/java/org/folio/spring/tenant/TestApplication.java create mode 100644 tenant/src/test/java/org/folio/spring/tenant/controller/TenantControllerTest.java create mode 100644 tenant/src/test/resources/application.yml diff --git a/pom.xml b/pom.xml index 5498dad..e22a319 100644 --- a/pom.xml +++ b/pom.xml @@ -81,6 +81,14 @@ 42.5.0 + + org.testcontainers + testcontainers-bom + 1.17.6 + import + pom + + diff --git a/tenant/pom.xml b/tenant/pom.xml index 2f700fe..93c5075 100644 --- a/tenant/pom.xml +++ b/tenant/pom.xml @@ -43,6 +43,12 @@ hibernate-c3p0 + + org.testcontainers + junit-jupiter + test + + diff --git a/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java b/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java index ad52309..d6c0131 100644 --- a/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java +++ b/tenant/src/main/java/org/folio/spring/tenant/hibernate/HibernateSchemaService.java @@ -88,7 +88,7 @@ public void afterPropertiesSet() throws Exception { String tenant = tenantProperties.getDefaultTenant(); Map settings = getSettings(tenant); try (Connection connection = getConnection(settings)) { - if (!schemaExists(connection, tenant)) { + if (!schemaExists(connection, settings)) { initializeSchema(connection, settings); } } @@ -98,7 +98,7 @@ public void afterPropertiesSet() throws Exception { public void createTenant(String tenant) throws SQLException, IOException { Map settings = getSettings(tenant); try (Connection connection = getConnection(settings)) { - if (schemaExists(connection, tenant)) { + if (schemaExists(connection, settings)) { throw new TenantAlreadyExistsException("Tenant already exists: " + tenant); } initializeSchema(connection, settings); @@ -108,7 +108,7 @@ public void createTenant(String tenant) throws SQLException, IOException { public void deleteTenant(String tenant) throws SQLException { Map settings = getSettings(tenant); try (Connection connection = getConnection(settings)) { - if (!schemaExists(connection, tenant)) { + if (!schemaExists(connection, settings)) { throw new TenantDoesNotExistsException("Tenant does not exist: " + tenant); } dropSchema(connection, getSchema(settings)); @@ -118,7 +118,7 @@ public void deleteTenant(String tenant) throws SQLException { public boolean schemaExists(String tenant) throws SQLException { Map settings = getSettings(tenant); try (Connection connection = getConnection(settings)) { - return schemaExists(connection, getSchema(settings)); + return schemaExists(connection, settings); } } @@ -175,10 +175,10 @@ private void dropSchema(Connection connection, String schema) throws SQLExceptio } } - private boolean schemaExists(Connection connection, String schema) throws SQLException { + private boolean schemaExists(Connection connection, Map settings) throws SQLException { String sql = "SELECT EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = ?)"; try (PreparedStatement statement = connection.prepareStatement(sql)) { - statement.setString(1, schema); + statement.setString(1, getSchema(settings)); ResultSet resultSet = statement.executeQuery(); if (resultSet.next()) { return resultSet.getBoolean(1); diff --git a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java index 0d72546..6cf0595 100644 --- a/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java +++ b/tenant/src/main/java/org/folio/spring/tenant/service/SchemaService.java @@ -1,5 +1,6 @@ package org.folio.spring.tenant.service; +import java.util.Locale; import java.util.regex.Pattern; import org.folio.spring.tenant.properties.BuildInfoProperties; import org.springframework.beans.factory.annotation.Autowired; @@ -8,14 +9,15 @@ @Service public class SchemaService { - // allowing only a-z A-Z 0-9 and _ prevents SQL injection - private static final Pattern SCHEMA_REGEXP = Pattern.compile("\\w+"); + // allow list of characters prevents SQL injection + private static final Pattern SCHEMA_REGEXP = Pattern.compile("[a-z0-9_]+"); @Autowired private BuildInfoProperties buildInfoProperties; public String getSchema(String tenant) { - var schema = String.format("%s_%s", tenant, buildInfoProperties.getArtifact()).replace("-", "_"); + String schema = String.format("%s_%s", tenant, buildInfoProperties.getArtifact()) + .replace("-", "_").toLowerCase(Locale.ROOT); if (! SCHEMA_REGEXP.matcher(schema).matches()) { throw new IllegalArgumentException("Illegal character in schema name: " + schema); } diff --git a/tenant/src/test/java/org/folio/spring/tenant/TestApplication.java b/tenant/src/test/java/org/folio/spring/tenant/TestApplication.java new file mode 100644 index 0000000..282638a --- /dev/null +++ b/tenant/src/test/java/org/folio/spring/tenant/TestApplication.java @@ -0,0 +1,20 @@ +package org.folio.spring.tenant; + +import org.springframework.boot.SpringApplication; +import org.springframework.boot.autoconfigure.SpringBootApplication; +import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.boot.web.servlet.support.SpringBootServletInitializer; + +@SpringBootApplication(scanBasePackages = "org.folio.spring") +public class TestApplication extends SpringBootServletInitializer { + + @Override + protected SpringApplicationBuilder configure(SpringApplicationBuilder application) { + return application.sources(TestApplication.class); + } + + public static void main(String[] args) { + SpringApplication.run(TestApplication.class, args); + } + +} diff --git a/tenant/src/test/java/org/folio/spring/tenant/controller/TenantControllerTest.java b/tenant/src/test/java/org/folio/spring/tenant/controller/TenantControllerTest.java new file mode 100644 index 0000000..728283e --- /dev/null +++ b/tenant/src/test/java/org/folio/spring/tenant/controller/TenantControllerTest.java @@ -0,0 +1,31 @@ +package org.folio.spring.tenant.controller; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.folio.spring.tenant.exception.TenantAlreadyExistsException; +import org.folio.spring.tenant.exception.TenantDoesNotExistsException; +import org.folio.spring.tenant.model.request.TenantAttributes; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; + +@SpringBootTest +class TenantControllerTest { + + @Autowired + TenantController tenantController; + + TenantAttributes tenantAttributes = new TenantAttributes("1.0.0"); + + @Test + void createAndDelete() throws Exception { + tenantController.create("a", tenantAttributes); + assertThrows(TenantAlreadyExistsException.class, () -> tenantController.create("a", tenantAttributes)); + tenantController.create("b", tenantAttributes); + tenantController.delete("b"); + assertThrows(TenantDoesNotExistsException.class, () -> tenantController.delete("b")); + assertThrows(TenantDoesNotExistsException.class, () -> tenantController.delete("c")); + tenantController.delete("a"); + } + +} diff --git a/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java b/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java index c423831..3bfe56a 100644 --- a/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java +++ b/tenant/src/test/java/org/folio/spring/tenant/service/SchemaServiceTest.java @@ -15,7 +15,7 @@ @SpringBootTest(classes = SchemaService.class) @EnableConfigurationProperties(value = BuildInfoProperties.class) -@TestPropertySource(properties = "info.build.artifact=mod-foo") +@TestPropertySource(properties = "info.build.artifact=mod-Baz") class SchemaServiceTest { @Autowired @@ -23,7 +23,7 @@ class SchemaServiceTest { @Test void validSchema() { - assertThat(schemaService.getSchema("tenant123"), is("tenant123_mod_foo")); + assertThat(schemaService.getSchema("FooBar123"), is("foobar123_mod_baz")); } @ParameterizedTest diff --git a/tenant/src/test/resources/application.yml b/tenant/src/test/resources/application.yml new file mode 100644 index 0000000..7e1cd96 --- /dev/null +++ b/tenant/src/test/resources/application.yml @@ -0,0 +1,53 @@ +info.build.artifact: mod-example + +logging: + file: logs/mod-workflow-tests.log + level: + org: + folio: INFO + hibernate: INFO + springframework: INFO + path: + +server: + port: 9101 + +spring: + application.name: mod-example + data.rest: + returnBodyOnCreate: true + returnBodyOnUpdate: true + datasource: + platform: h2 + url: jdbc:h2:mem:AZ;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE;MODE=PostgreSQL;DATABASE_TO_LOWER=true;DEFAULT_NULL_ORDERING=HIGH + driverClassName: org.h2.Driver + username: folio + password: folio + h2: + console: + enabled: true + path: /h2console + jpa: + database-platform: org.hibernate.dialect.H2Dialect + properties.hibernate.jdbc.lob.non_contextual_creation: true + generate-ddl: false + hibernate.ddl-auto: none + open-in-view: true + show-sql: false + profiles: + active: default + thymeleaf: + mode: TEXT + suffix: .sql + +event.queue.name: event.queue + +tenant: + header-name: X-Okapi-Tenant + force-tenant: false + default-tenant: diku + initialize-default-tenant: true + domain-packages: + schema-scripts: + +okapi.url: http://localhost:9130 From da296abcee6815f49665f87928a8143497c6420d Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Wed, 23 Nov 2022 16:02:28 +0100 Subject: [PATCH 5/5] Remove unused testcontainers --- pom.xml | 8 -------- tenant/pom.xml | 6 ------ 2 files changed, 14 deletions(-) diff --git a/pom.xml b/pom.xml index e22a319..5498dad 100644 --- a/pom.xml +++ b/pom.xml @@ -81,14 +81,6 @@ 42.5.0 - - org.testcontainers - testcontainers-bom - 1.17.6 - import - pom - - diff --git a/tenant/pom.xml b/tenant/pom.xml index 93c5075..2f700fe 100644 --- a/tenant/pom.xml +++ b/tenant/pom.xml @@ -43,12 +43,6 @@ hibernate-c3p0 - - org.testcontainers - junit-jupiter - test - -