launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/strict-commissioning-script-names into lp:maas.

Commit message:
Ban whitespace and quotes from commissioning-script names.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/strict-commissioning-script-names/+merge/140831

This is pursuant to https://code.launchpad.net/~jtv/maas/report-script-result/+merge/140650

The main commissioning script deals with names of commissioning scripts as downloaded from the server.  The quoting in that shell code could get horribly confused if there are any "weird" characters in the names, and we don't currently have a way of unit-testing it.

But we already document a restrictive naming convention, with examples that make it unlikely that users would even think of doing such things.  So we might as well make that official: this branch makes it impossible to include confusing characters in commissioning-script names.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/strict-commissioning-script-names/+merge/140831
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/strict-commissioning-script-names into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-12-12 17:12:25 +0000
+++ src/maasserver/api.py	2012-12-20 08:08:19 +0000
@@ -1797,7 +1797,8 @@
         By convention the name should consist of a two-digit number, a dash,
         and a brief descriptive identifier consisting only of ASCII
         characters.  You don't need to follow this convention, but not doing
-        so opens you up to risks w.r.t. encoding and ordering.
+        so opens you up to risks w.r.t. encoding and ordering.  The name must
+        not contain any whitespace, quotes, or apostrophes.
 
         A commissioning node will run each of the scripts in lexicographical
         order.  There are no promises about how non-ASCII characters are
@@ -1815,7 +1816,7 @@
 
         :param name: Unique identifying name for the script.  Names should
             follow the pattern of "25-burn-in-hard-disk" (all ASCII, and with
-            numbers greater than zero).
+            numbers greater than zero, and generally no "weird" characters).
         :param content: A script file, to be uploaded in binary form.  Note:
             this is not a normal parameter, but a file upload.  Its filename
             is ignored; MAAS will know it by the name you pass to the request.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-11-30 17:21:28 +0000
+++ src/maasserver/forms.py	2012-12-20 08:08:19 +0000
@@ -879,6 +879,10 @@
     def clean_content(self):
         content = self.cleaned_data['content']
         name = content.name
+        if re.search('\s', name):
+            raise forms.ValidationError("Name contains whitespace.")
+        if re.search('["\']', name):
+            raise forms.ValidationError("Name contains quote or apostrophe.")
         if CommissioningScript.objects.filter(name=name).exists():
             raise forms.ValidationError(
                 "A script with that name already exists.")

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-11-30 17:21:28 +0000
+++ src/maasserver/tests/test_forms.py	2012-12-20 08:08:19 +0000
@@ -970,6 +970,23 @@
         uploaded_file = SimpleUploadedFile(content=content, name=name)
         form = CommissioningScriptForm(files={'content': uploaded_file})
         self.assertEqual(
-            (False, {'content':
-                [u'A script with that name already exists.']}),
+            (False, {'content': ["A script with that name already exists."]}),
+            (form.is_valid(), form._errors))
+
+    def test_rejects_whitespace_in_name(self):
+        name = factory.make_name('with space')
+        uploaded_file = SimpleUploadedFile(
+            content=factory.getRandomString(), name=name)
+        form = CommissioningScriptForm(files={'content': uploaded_file})
+        self.assertEqual(
+            (False, {'content': ["Name contains whitespace."]}),
+            (form.is_valid(), form._errors))
+
+    def test_rejects_quotes_in_name(self):
+        name = factory.make_name("l'horreur")
+        uploaded_file = SimpleUploadedFile(
+            content=factory.getRandomString(), name=name)
+        form = CommissioningScriptForm(files={'content': uploaded_file})
+        self.assertEqual(
+            (False, {'content': ["Name contains quote or apostrophe."]}),
             (form.is_valid(), form._errors))