Content security policy for Django-served pages (#639)

* Add django_csp to requirements.txt. * s/csp_nonce/nonce This is to be consistent with what the django_csp library calls it. * Revert "s/csp_nonce/nonce" This reverts commit f711284. * Set up django_csp. * Add some CSP nonces. * CSP nonces for side menu showing/hiding. * admin-base CSP fixes. * Fix onload function. * CSP settings. * Try upgrading pip. * Try setting SERVER_SOFTWARE in tools/lint. * export
google · Apr 15, 2019 · 6297daa · 6297daa
1 parent 91a11be
commit 6297daa
Show file tree

Hide file tree

Showing 12 changed files with 51 additions and 14 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -26,6 +26,10 @@ install:
       git \
       time \
       unzip
+  # The default version of pip will try to install the latest version of Django
+  # (apparently as part of installing django_csp), which doesn't work because
+  # the latest version of Django requires Python 3.
+  - python -m pip install --upgrade pip setuptools
   - wget https://storage.googleapis.com/appengine-sdks/featured/google_appengine_1.9.83.zip -O /tmp/appengine.zip
   - unzip -qq /tmp/appengine.zip
   - |

diff --git a/app/admin.py b/app/admin.py
@@ -64,7 +64,7 @@ def get(self):
                     login_url=users.create_login_url(self.request.url),
                     logout_url=users.create_logout_url(self.request.url),
                     language_exonyms_json=sorted_exonyms_json,
-                    onload_function="add_initial_languages()",
+                    onload_function="add_initial_languages",
                     id=self.env.domain + '/person.',
                     test_mode_min_age_hours=
                         tasks.CleanUpInTestMode.DELETION_AGE_SECONDS / 3600.0,

diff --git a/app/create.py b/app/create.py
@@ -47,7 +47,7 @@ def get(self):
         self.render('create.html',
                     profile_websites=profile_websites,
                     profile_websites_json=simplejson.dumps(profile_websites),
-                    onload_function='view_page_loaded()')
+                    onload_function='view_page_loaded')
 
     def post(self):
         now = get_utcnow()

diff --git a/app/multiview.py b/app/multiview.py
@@ -75,7 +75,7 @@ def get(self):
         self.render('multiview.html',
                     person=person, any=any_person,
                     cols=len(person['full_name']) + 1,
-                    onload_function='view_page_loaded()', markdup=True,
+                    onload_function='view_page_loaded', markdup=True,
                     show_private_info=show_private_info, reveal_url=reveal_url)
 
     def post(self):

diff --git a/app/resources/admin-base.html.template b/app/resources/admin-base.html.template
@@ -19,11 +19,14 @@
 {% endblock %}
 
 {% block logo %}
-  <img class="title-bar-button" id="menu-button" src="{{env.global_url}}/menu.png" alt="Menu" onclick="showSideMenu()">{% trans "Google Person Finder" %}
+  <img class="title-bar-button" id="menu-button" src="{{env.global_url}}/menu.png" alt="Menu">{% trans "Google Person Finder" %}
+  <script type="text/javascript" nonce="{{csp_nonce}}">
+    document.querySelector('#menu-button').addEventListener('click', showSideMenu);
+  </script>
 {% endblock %}
 
 {% block body %}
-  <script>
+  <script nonce="{{csp_nonce}}">
     function $(id) {
       return document.getElementById(id);
     }
@@ -52,8 +55,8 @@
     <a href="{{env.global_url}}/admin/statistics">Historical statistics</a>
 
   <h2>Repository</h2>
-  <form method="get" style="display: inline-block" class="admin">
-    <select name="repo" id="select_repo" onchange="select_repo_changed()">
+  <form method="get" class="admin" id="admin-reposelector">
+    <select name="repo" id="select_repo">
       {% if not env.repo %}
         <option>Select a repository:</option>
       {% endif %}
@@ -69,6 +72,10 @@
       <option value="__new__">Create new...</option>
     </select>
   </form>
+  <script type="text/javascript" nonce="{{csp_nonce}}">
+    document.getElementById('select_repo').addEventListener(
+        'change', select_repo_changed);
+  </script>
 
   {% if env.repo %}
     <a href="{{env.repo_url}}/admin">Repository settings</a>

diff --git a/app/resources/base.html.template b/app/resources/base.html.template
@@ -74,22 +74,27 @@
        rel="stylesheet"
        href="{{env.global_url}}/css?lang={{env.lang}}&ui={{env.ui}}">
     {% if env.enable_javascript %}
-      <script type="text/javascript">
+      <script type="text/javascript" nonce="{{csp_nonce}}">
         var lang = '{{env.lang}}';
         {% if env.enable_translate %}
           var translate_api_key = '{{config.translate_api_key}}';
         {% else %}
           var translate_api_key = null;
         {% endif %}
       </script>
-      <script src="{{env.global_url}}/forms.js"></script>
+      <script src="{{env.global_url}}/forms.js" nonce="{{csp_nonce}}"></script>
     {% endif %}
   {% endif %}
+  {% if onload_function and not env.amp %}
+    <script type="text/javascript" nonce="{{csp_nonce}}">
+      document.addEventListener('DOMContentLoaded', function() {
+        {{onload_function}}();
+      });
+    </script>
+  {% endif %}
 {% endblock head %}</head>
 
-<body
-    class="{{env.ui}} {% block extra_body_class %}{% endblock %}"
-    {% if not env.amp %}onload="{{onload_function}}"{% endif %}>
+<body class="{{env.ui}} {% block extra_body_class %}{% endblock %}">
   {% block body %}
     {% if env.amp and env.enable_analytics %}
       <amp-analytics config="https://www.googletagmanager.com/amp.json?id={{config.amp_gtm_id}}"
@@ -207,7 +212,10 @@
               >{% trans "Terms of Service" %}</a>
         {% endblock sidenav %}
       </div>
-      <div id="overlay" class="overlay" onclick="hideSideMenu()"></div>
+      <div id="overlay" class="overlay"></div>
+      <script type="text/javascript" nonce="{{csp_nonce}}">
+        document.querySelector('#overlay').addEventListener('click', hideSideMenu);
+      </script>
     {% endif %}
   {% endblock body %}
 </body></html>
diff --git a/app/resources/css-default.template b/app/resources/css-default.template
@@ -1080,6 +1080,10 @@ form.admin .config .note {
   margin-{{start}}: 4em;
 }
 
+#admin-reposelector {
+  display: inline-block;
+}
+
 .subscribe_email_error {
   color: #f00;
 }

diff --git a/app/settings.py b/app/settings.py
@@ -54,6 +54,7 @@
     'django.middleware.locale.LocaleMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.middleware.clickjacking.XFrameOptionsMiddleware',
+    'csp.middleware.CSPMiddleware',
 ]
 
 ROOT_URLCONF = 'urls'
@@ -70,6 +71,14 @@
   SECURE_REDIRECT_EXEMPT += [
       r'^%s/.*/tasks/.*' % site_settings.OPTIONAL_PATH_PREFIX]
 
+# Based on the Strict CSP example here:
+# https://csp.withgoogle.com/docs/strict-csp.html
+CSP_INCLUDE_NONCE_IN = ('script-src',)
+CSP_BASE_URI = "'none'"
+CSP_OBJECT_SRC = "'none'"
+CSP_SCRIPT_SRC = ("'unsafe-inline'", "'unsafe-eval'",
+                  "'strict-dynamic' https: http:",)
+
 TEMPLATES = [
     {
         'BACKEND': 'django.template.backends.django.DjangoTemplates',

diff --git a/app/view.py b/app/view.py
@@ -152,7 +152,7 @@ def get(self):
                     person=person,
                     notes=notes,
                     linked_person_info=linked_person_info,
-                    onload_function='view_page_loaded()',
+                    onload_function='view_page_loaded',
                     show_private_info=show_private_info,
                     admin=users.is_current_user_admin(),
                     dupe_notes_url=dupe_notes_url,

diff --git a/app/views/base.py b/app/views/base.py
@@ -263,6 +263,7 @@ def get_vars():
             # already has the config anyway
             template_vars['config'] = self.env.config
             template_vars['params'] = self.params
+            template_vars['csp_nonce'] = self.request.csp_nonce
             return template_vars
 
         query_str = self.request.META.get('QUERY_STRING', '')

diff --git a/requirements.txt b/requirements.txt
@@ -1,5 +1,6 @@
 Babel==2.3.4
 defusedxml==0.5.0
+django_csp==3.5
 google-api-python-client==1.7.8
 oauth2client==4.1.3
 pytz==2016.10

diff --git a/tools/lint b/tools/lint
@@ -36,6 +36,9 @@ if [ "$command" == "yapf-fix" ]; then
 elif [ "$command" == "yapf-check" ]; then
     $YAPF -d $file_list
 elif [ "$command" == "pylint-check" ]; then
+    # Pylint apparently runs some stuff, including App Engine code that expects
+    # this to be set.
+    export SERVER_SOFTWARE="testing"
     $PYLINT $file_list
 else
     echo "Usage: tools/lint [yapf-fix|yapf-check|pylint-check]"