Skip to content

Commit f83792b

Browse files
GSPattonGSPatton
authored
Merge pull request #1484 from puppetlabs/maint-harden_db_defined_type
Harden db defined type
2 parents e70e7fd + cd2e485 commit f83792b

File tree

4 files changed

+111
-32
lines changed

4 files changed

+111
-32
lines changed

‎manifests/db.pp‎

Lines changed: 43 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@
99
# grant => ['SELECT', 'UPDATE'],
1010
# }
1111
#
12+
# @param name
13+
# The name of the database to create. Database names must:
14+
# * be longer than 64 characters.
15+
# * not contain / \ or . characters.
16+
# * not contain characters that are not permitted in file names.
17+
# * not end with space characters.
1218
# @param user
1319
# The user for the database you're creating.
1420
# @param password
@@ -28,7 +34,7 @@
2834
# @param grant_options
2935
# The grant_options for the grant for user@host on the database.
3036
# @param sql
31-
# The path to the sqlfile you want to execute. This can be single file specified as string, or it can be an array of strings.
37+
# The path to the sqlfile you want to execute. This can be a an array containing one or more file paths.
3238
# @param enforce_sql
3339
# Specifies whether executing the sqlfiles should happen on every run. If set to false, sqlfiles only run once.
3440
# @param ensure
@@ -41,25 +47,41 @@
4147
define mysql::db (
4248
$user,
4349
Variant[String, Sensitive[String]] $password,
44-
$tls_options = undef,
45-
$dbname = $name,
46-
$charset = 'utf8',
47-
$collate = 'utf8_general_ci',
48-
$host = 'localhost',
49-
$grant = 'ALL',
50-
$grant_options = undef,
51-
Optional[Variant[Array, Hash, String]] $sql = undef,
52-
$enforce_sql = false,
53-
Enum['absent', 'present'] $ensure = 'present',
54-
$import_timeout = 300,
55-
$import_cat_cmd = 'cat',
56-
$mysql_exec_path = undef,
50+
$tls_options = undef,
51+
String $dbname = $name,
52+
$charset = 'utf8',
53+
$collate = 'utf8_general_ci',
54+
$host = 'localhost',
55+
$grant = 'ALL',
56+
$grant_options = undef,
57+
Optional[Array] $sql = undef,
58+
$enforce_sql = false,
59+
Enum['absent', 'present'] $ensure = 'present',
60+
$import_timeout = 300,
61+
Enum['cat', 'zcat', 'bzcat'] $import_cat_cmd = 'cat',
62+
$mysql_exec_path = undef,
5763
) {
58-
$table = "${dbname}.*"
64+
include 'mysql::client'
5965

60-
$sql_inputs = join([$sql], ' ')
66+
# Ensure that the database name is valid.
67+
if $dbname !~ /^[^\/?%*:|\""<>.\s;]{1,64}$/ {
68+
$message = "The database name '${dbname}' is invalid. Values must:
69+
* be longer than 64 characters.
70+
* not contain // \\ or . characters.
71+
* not contain characters that are not permitted in file names.
72+
* not end with space characters."
73+
fail($message)
74+
}
6175

62-
include 'mysql::client'
76+
# Ensure that the sql files passed are valid file paths.
77+
if $sql {
78+
$sql.each | $sqlfile | {
79+
if $sqlfile !~ /^\/(?:[A-Za-z0-9_-]+\/?+)+(?:.[A-Za-z0-9]+)$/ {
80+
$message = "The file '${sqlfile}' is invalid. A a valid file path is expected."
81+
fail($message)
82+
}
83+
}
84+
}
6385

6486
if ($mysql_exec_path) {
6587
$_mysql_exec_path = $mysql_exec_path
@@ -84,6 +106,8 @@
84106
ensure_resource('mysql_user', "${user}@${host}", $user_resource)
85107

86108
if $ensure == 'present' {
109+
$table = "${dbname}.*"
110+
87111
mysql_grant { "${user}@${host}/${table}":
88112
privileges => $grant,
89113
provider => 'mysql',
@@ -96,14 +120,12 @@
96120
],
97121
}
98122

99-
$refresh = ! $enforce_sql
100-
101123
if $sql {
102124
exec { "${dbname}-import":
103-
command => "${import_cat_cmd} ${sql_inputs} | mysql ${dbname}",
125+
command => "${import_cat_cmd} ${shell_join($sql)} | mysql ${dbname}",
104126
logoutput => true,
105127
environment => "HOME=${::root_home}",
106-
refreshonly => $refresh,
128+
refreshonly => ! $enforce_sql,
107129
path => "/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:${_mysql_exec_path}",
108130
require => Mysql_grant["${user}@${host}/${table}"],
109131
subscribe => Mysql_database[$dbname],

‎spec/acceptance/01_mysql_db_spec.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } }
4343
mysql::db { 'spec2':
4444
user => 'root1',
4545
password => 'password',
46-
sql => '/tmp/spec.sql',
4746
charset => '#{fetch_charset}',
4847
collate => '#{fetch_charset}_general_ci',
48+
sql => ['/tmp/spec.sql'],
4949
}
5050
MANIFEST
5151
end

‎spec/acceptance/types/mysql_grant_spec.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -686,8 +686,8 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } }
686686
mysql::db { 'grant_spec_db':
687687
user => 'root1',
688688
password => 'password',
689-
sql => '/tmp/grant_spec_table.sql',
690689
charset => '#{fetch_charset}',
690+
sql => ['/tmp/grant_spec_table.sql'],
691691
}
692692
MANIFEST
693693
it 'creates table' do

‎spec/defines/mysql_db_spec.rb‎

Lines changed: 66 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,40 +17,42 @@
1717
'mysql_exec_path' => '' }
1818
end
1919

20+
let(:sql) { [ '/tmp/test.sql' ] }
21+
2022
it 'does not notify the import sql exec if no sql script was provided' do
2123
is_expected.to contain_mysql_database('test_db').without_notify
2224
end
2325

2426
it 'subscribes to database if sql script is given' do
25-
params['sql'] = 'test_sql'
27+
params['sql'] = sql
2628
is_expected.to contain_mysql_database('test_db')
2729
is_expected.to contain_exec('test_db-import').with_subscribe('Mysql_database[test_db]')
2830
end
2931

3032
it 'onlies import sql script on creation if not enforcing' do
31-
params.merge!('sql' => 'test_sql', 'enforce_sql' => false)
33+
params.merge!('sql' => sql, 'enforce_sql' => false)
3234
is_expected.to contain_exec('test_db-import').with_refreshonly(true)
3335
end
3436

3537
it 'imports sql script on creation' do
36-
params.merge!('sql' => 'test_sql', 'enforce_sql' => true)
38+
params.merge!('sql' => sql, 'enforce_sql' => true)
3739
# ' if enforcing #refreshonly'
3840
is_expected.to contain_exec('test_db-import').with_refreshonly(false)
3941
# 'if enforcing #command'
40-
is_expected.to contain_exec('test_db-import').with_command('cat test_sql | mysql test_db')
42+
is_expected.to contain_exec('test_db-import').with_command('cat /tmp/test.sql | mysql test_db')
4143
end
4244

43-
it 'imports sql script with custom command on creation ' do
44-
params.merge!('sql' => 'test_sql', 'enforce_sql' => true, 'import_cat_cmd' => 'zcat')
45+
it 'imports sql script with custom command on creation' do
46+
params.merge!('sql' => sql, 'enforce_sql' => true, 'import_cat_cmd' => 'zcat')
4547
# if enforcing #refreshonly
4648
is_expected.to contain_exec('test_db-import').with_refreshonly(false)
4749
# if enforcing #command
48-
is_expected.to contain_exec('test_db-import').with_command('zcat test_sql | mysql test_db')
50+
is_expected.to contain_exec('test_db-import').with_command('zcat /tmp/test.sql | mysql test_db')
4951
end
5052

5153
it 'imports sql scripts when more than one is specified' do
52-
params['sql'] = ['test_sql', 'test_2_sql']
53-
is_expected.to contain_exec('test_db-import').with_command('cat test_sql test_2_sql | mysql test_db')
54+
params['sql'] = ['/tmp/test.sql', '/tmp/test_2.sql']
55+
is_expected.to contain_exec('test_db-import').with_command('cat /tmp/test.sql /tmp/test_2.sql | mysql test_db')
5456
end
5557

5658
it 'does not create database' do
@@ -79,6 +81,61 @@
7981
params['grant_options'] = ['GRANT']
8082
is_expected.to contain_mysql_grant('testuser@localhost/test_db.*').with_options(['GRANT'])
8183
end
84+
85+
# Invalid file paths
86+
[
87+
'|| ls -la ||',
88+
'|| touch /tmp/foo.txt ||',
89+
'/tmp/foo.txt;echo',
90+
'myPath;',
91+
'\\myPath\\',
92+
'//myPath has spaces//',
93+
'/',
94+
].each do |path|
95+
it "fails when provided '#{path}' as a value to the 'sql' parameter" do
96+
params['sql'] = [path]
97+
is_expected.to raise_error(Puppet::PreformattedError, %r{The file '#{Regexp.escape(path)}' is invalid. A a valid file path is expected.})
98+
end
99+
end
100+
101+
# Valid file paths
102+
[
103+
'/tmp/test.txt',
104+
'/tmp/.test',
105+
'/foo.test',
106+
].each do |path|
107+
it "succeeds when provided '#{path}' as a value to the 'sql' parameter" do
108+
params['sql'] = [path]
109+
is_expected.to contain_exec('test_db-import').with_command("cat #{path} | mysql test_db")
110+
end
111+
end
112+
113+
# Invalid database names
114+
[
115+
'test db',
116+
'test_db;',
117+
'test/db',
118+
'|| ls -la ||',
119+
'|| touch /tmp/foo.txt ||',
120+
].each do |name|
121+
it "fails when provided '#{name}' as a value to the 'name' parameter" do
122+
params['name'] = name
123+
is_expected.to raise_error(Puppet::PreformattedError, %r{The database name '#{name}' is invalid.})
124+
end
125+
end
126+
127+
# Valid database names
128+
[
129+
'test_db',
130+
'testdb',
131+
'test-db',
132+
'TESTDB',
133+
].each do |name|
134+
it "succeeds when the provided '#{name}' as a value to the 'dbname' parameter" do
135+
params['dbname'] = name
136+
is_expected.to contain_mysql_database(name)
137+
end
138+
end
82139
end
83140
end
84141
end

0 commit comments

Comments
 (0)