Merge lp:~bloodearnest/django-preflight/optional-settings into lp:django-preflight

[Simon Davy]

- Allow disabling setting dump on overview page.
- Default PREFLIGHT_ENABLE_SETTINGS to False, and improve secret cleansing.

Make settings optional via config

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[Natalia Bidart (nataliabidart)]

Branch looks good, but can we please make the default be False?

[Natalia Bidart (nataliabidart)]

Expanding a bit on my former comment: having settings enabled by default makes much easier to expose sensitive data through the preflight overview page, because the gather_settings function is not smart about hidding settings inside a dict.

So, if we will leave the "show settings" defaulting to True, I would please request that preflight will copy/reuse the logic in django/views/debug.py:26:

def cleanse_setting(key, value):
    """Cleanse an individual setting key/value of sensitive content.

    If the value is a dictionary, recursively cleanse the keys in
    that dictionary.
    """
    try:
        if HIDDEN_SETTINGS.search(key):
            cleansed = '********************'
        else:
            if isinstance(value, dict):
                cleansed = dict((k, cleanse_setting(k, v)) for k,v in value.items())
            else:
                cleansed = value
    except TypeError:
        # If the key isn't regex-able, just return as-is.
        cleansed = value
    return cleansed

review: Needs Fixing

[lp:~bloodearnest/django-preflight/optional-settings]

29. By Simon Davy on 2013-06-18

Default PREFLIGHT_ENABLE_SETTINGS to False, and improve secret cleansing

[Łukasz Czyżykowski (lukasz-czyzykowski)]

review: Approve

[Natalia Bidart (nataliabidart)]

09:35 < nessita> bloodearnest, branch looks great, I'm approving. Any chance you can add a test where a setting inside a dict is expected to be masked?
09:35 < nessita> such as the database setting
09:39 < bloodearnest> nessita, good idea, will do
09:39 < nessita> bloodearnest, thanks, will leave my vote in there

review: Approve

[lp:~bloodearnest/django-preflight/optional-settings]

30. By Simon Davy on 2013-06-18

added test for new cleansing code

31. By Simon Davy on 2013-06-18

minor tweak for django 1.1/1.2 support

=== modified file 'preflight/conf.py'
--- preflight/conf.py	2013-05-17 14:51:29 +0000
+++ preflight/conf.py	2013-06-18 13:38:25 +0000
@@ -4,3 +4,4 @@
 BASE_TEMPLATE = getattr(
     settings, 'PREFLIGHT_BASE_TEMPLATE', "admin/base.html")
 TABLE_CLASS = getattr(settings, 'PREFLIGHT_TABLE_CLASS', "listing")
+ENABLE_SETTINGS = getattr(settings, 'PREFLIGHT_ENABLE_SETTINGS', False)

=== modified file 'preflight/models.py'
--- preflight/models.py	2013-06-10 12:57:48 +0000
+++ preflight/models.py	2013-06-18 13:38:25 +0000
@@ -8,6 +8,8 @@
 from django.conf import settings
 from django.views.debug import HIDDEN_SETTINGS
 
+from .conf import ENABLE_SETTINGS
+
 REGISTRY = []
 
 
@@ -130,7 +132,31 @@
         return "not found"
 
 
+# taken from django.debug.views.py at 7902fd74f1
+def cleanse_setting(key, value, hidden):
+    """Cleanse an individual setting key/value of sensitive content.
+
+    If the value is a dictionary, recursively cleanse the keys in
+    that dictionary.
+    """
+    try:
+        if hidden.search(key):
+            cleansed = '******************'
+        else:
+            if isinstance(value, dict):
+                cleansed = dict((k, cleanse_setting(k, v, hidden))
+                                for k,v in value.items())
+            else:
+                cleansed = value
+    except TypeError:
+        # If the key isn't regex-able, just return as-is.
+        cleansed = value
+    return cleansed
+
 def gather_settings():
+    if not ENABLE_SETTINGS:
+        return []
+
     names = sorted([x for x in settings._wrapped.__dict__
         if x.isupper() and not x.startswith('_')])
     settings_list = []
@@ -140,13 +166,10 @@
     else:
         hidden_settings = HIDDEN_SETTINGS
     for name in names:
-        if hidden_settings.search(name):
-            value = '******************'
-        else:
-            value = getattr(settings, name, None)
+        value = getattr(settings, name, None)
         settings_list.append({
             'name': name,
-            'value': value,
+            'value': cleanse_setting(name, value, hidden_settings),
             'location': locate_setting(name),
         })
     return settings_list

=== modified file 'preflight/templates/preflight/overview.html'
--- preflight/templates/preflight/overview.html	2013-06-04 15:00:12 +0000
+++ preflight/templates/preflight/overview.html	2013-06-18 13:38:25 +0000
@@ -16,7 +16,9 @@
       <li><a href="#{{ application.name|slugify }}">{{ application.name }}</a></li>
     {% endfor %}
     <li><a href="#versions">Versions</a></li>
+    {% if settings %}
     <li><a href="#settings">Settings</a></li>
+    {% endif %}
     {% if switches %}
     <li><a href="#switches">Switches</a></li>
     {% endif %}
@@ -72,6 +74,7 @@
     </tbody>
   </table>
 
+  {% if settings %}
   <a href="#links" style="text-decoration: none;">
     <h1 id="settings">{% trans "Settings" %}</h1>
   </a>
@@ -82,6 +85,7 @@
     {% if setting.location %}<dd>Location: {{ setting.location}}</dd>{% endif %}
   {% endfor %}
   </dl>
+  {% endif %}
 
   {% if switches %}
   <a href="#links" style="text-decoration: none;">

=== modified file 'preflight/tests.py'
--- preflight/tests.py	2013-06-10 12:57:48 +0000
+++ preflight/tests.py	2013-06-18 13:38:25 +0000
@@ -26,8 +26,10 @@
 from .models import (
     Application,
     Check,
+    HIDDEN_SETTINGS,
     REGISTRY,
     authenticate,
+    cleanse_setting,
     gather_checks,
     gather_switches,
     gather_gargoyle,
@@ -322,6 +324,8 @@
 
 
 class SettingsTestCase(TestCase):
+
+    @patch('preflight.models.ENABLE_SETTINGS', True)
     @patch('preflight.models.settings')
     def test_gather_settings_no_location(self, mock_settings):
         mock_settings._wrapped.FOO = 'bar'
@@ -331,6 +335,7 @@
         expected = [{'name': 'FOO', 'value': 'bar', 'location': ''}]
         self.assertEqual(expected, settings)
 
+    @patch('preflight.models.ENABLE_SETTINGS', True)
     @patch('preflight.models.settings')
     def test_gather_settings_with_location(self, mock_settings):
         mock_settings._wrapped.FOO = 'bar'
@@ -343,6 +348,7 @@
         expected = [{'name': 'FOO', 'value': 'bar', 'location': '/tmp/baz'}]
         self.assertEqual(expected, settings)
 
+    @patch('preflight.models.ENABLE_SETTINGS', True)
     @patch('preflight.models.settings')
     def test_gather_settings_hidden(self, mock_settings):
         mock_settings._wrapped.FOO_ZOGGLES = 'bar'
@@ -352,6 +358,7 @@
         expected = [{'name': 'FOO_ZOGGLES', 'value': '*' * 18, 'location': ''}]
         self.assertEqual(expected, settings)
 
+    @patch('preflight.models.ENABLE_SETTINGS', True)
     @patch('preflight.models.settings')
     def test_gather_settings_omitted(self, mock_settings):
         mock_settings._wrapped._FOO = 'bar'
@@ -360,6 +367,31 @@
         settings = gather_settings()
         self.assertEqual([], settings)
 
+    def test_gather_settings_disabled(self):
+        self.assertEqual(gather_settings(), [])
+
+    def test_gargoyle_template_no_settings(self):
+        context = {
+            "settings": [],
+            "preflight_base_template": BASE_TEMPLATE,
+            "preflight_table_class": TABLE_CLASS,
+        }
+        response = render_to_string('preflight/overview.html', context)
+        dom = PyQuery(response)
+
+        settings_header = dom.find('#settings')
+        self.assertEqual(settings_header, [])
+
+    def test_cleanse_setting(self):
+        key = 'DATABASES'
+        value = {
+            'default': {
+                'PASSWORD': 'secret'
+            }
+        }
+        result = cleanse_setting(key, value, HIDDEN_SETTINGS)
+        self.assertFalse('secret' in result['default']['PASSWORD'])
+
 
 @skipIf(not settings.USE_GARGOYLE, 'skipping for Django 1.1')
 class GargoyleTestCase(TestCase):