Merge pull request #38 from plone/maurits-issue-2813-plone-protect

Only keep already uploaded image when it is a POST request
plone · Mar 26, 2019 · 846f5f4 · 846f5f4
2 parents 02b8b04 + 3975588
commit 846f5f4
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 26 deletions.
diff --git a/news/2628.bugfix b/news/2628.bugfix
@@ -0,0 +1,4 @@
+Only use the new feature for keeping an already uploaded image when it is a POST request.
+Fixes auto csrf error in `site-controlpanel <https://github.com/plone/Products.CMFPlone/issues/2628>`_
+and `personal-information <https://github.com/plone/Products.CMFPlone/issues/2709>`_ page.
+[maurits]
diff --git a/plone/formwidget/namedfile/tests.py b/plone/formwidget/namedfile/tests.py
@@ -11,8 +11,19 @@
 class Py23DocChecker(doctest.OutputChecker):
     def check_output(self, want, got, optionflags):
         if six.PY2:
-            got = re.sub('zope.publisher.interfaces.NotFound', 'NotFound', got)
-            got = re.sub("u'(.*?)'", "'\\1'", want)
+            got = re.sub('NotFound', 'zope.publisher.interfaces.NotFound', got)
+            got = re.sub('InvalidState', 'plone.formwidget.namedfile.validator.InvalidState', got)
+            got = re.sub('RequiredMissing', 'zope.schema._bootstrapinterfaces.RequiredMissing', got)
+            got = re.sub('IOError: cannot identify image file', 'OSError: cannot identify image file', got)
+            got = re.sub('IO instance', 'IO object', got)
+            got = re.sub("u'(.*?)'", "'\\1'", got)
+            got = re.sub('u"(.*?)"', '"\\1"', got)
+            got = re.sub("b'(.*?)'", "'\\1'", got)
+            got = re.sub('b"(.*?)"', '"\\1"', got)
+            want = re.sub("u'(.*?)'", "'\\1'", want)
+            want = re.sub('u"(.*?)"', '"\\1"', want)
+            want = re.sub("b'(.*?)'", "'\\1'", want)
+            want = re.sub('b"(.*?)"', '"\\1"', want)
         return doctest.OutputChecker.check_output(self, want, got, optionflags)
 
 

diff --git a/plone/formwidget/namedfile/widget.py b/plone/formwidget/namedfile/widget.py
@@ -98,7 +98,21 @@ def file_upload_id(self):
         """Temporary store the uploaded file contents with a file_upload_id key.
         In case of form validation errors the already uploaded image can then
         be reused.
+
+        This is only useful on a POST request:
+        forms should not be using GET,
+        especially when you save something to the database.
+
+        Note that if we want this on a GET request,
+        we should add a safeWrite call in the code below:
+        plone.protect.utils.safeWrite(up.upload_map, self.request)
+        Otherwise plone.protect auto csrf will complain for example
+        when getting @@site-controlpanel or @@personal-information
+        See https://github.com/plone/Products.CMFPlone/issues/2628
+        and https://github.com/plone/Products.CMFPlone/issues/2709
         """
+        if self.request.method != 'POST':
+            return ''
         if self._file_upload_id:
             # cache this property for multiple calls within one request.
             # This avoids storing a file upload multiple times.
@@ -270,7 +284,8 @@ def extract(self, default=NOVALUE):
                 data = fileinfo.get('data')
 
                 if filename or data:
-                    filename = safe_basename(filename)
+                    if filename:
+                        filename = safe_basename(filename)
                     if (
                             filename is not None
                             and not isinstance(filename, six.text_type)

diff --git a/plone/formwidget/namedfile/widget.rst b/plone/formwidget/namedfile/widget.rst
@@ -31,20 +31,31 @@ There are also more specific interfaces for each widget::
 The widgets can be instantiated only using the request::
 
   >>> from z3c.form.testing import TestRequest
+  >>> def make_get_request(**kwargs):
+  ...     # GET is actually the default.
+  ...     # return TestRequest(**kwargs)
+  ...     req = TestRequest(**kwargs)
+  ...     req.method = 'GET'
+  ...     return req
   >>> def make_request(**kwargs):
-  ...     return TestRequest(**kwargs)
+  ...     req = TestRequest(**kwargs)
+  ...     req.method = 'POST'
+  ...     return req
   >>> request = make_request()
-
-  >>> file_widget = NamedFileWidget(request)
-  >>> image_widget = NamedImageWidget(request)
-
-Before rendering a widget, one has to set the name and id of the widget::
-
-  >>> file_widget.id = 'widget.id.file'
-  >>> file_widget.name = 'widget.name.file'
-
-  >>> image_widget.id = 'widget.id.image'
-  >>> image_widget.name = 'widget.name.image'
+  >>> def make_file_widget(request):
+  ...     file_widget = NamedFileWidget(request)
+  ...     # In some versions, before rendering a widget, one has to set the name and id of the widget:
+  ...     file_widget.id = 'widget.id.file'
+  ...     file_widget.name = 'widget.name.file'
+  ...     return file_widget
+  >>> def make_image_widget(request):
+  ...     image_widget = NamedImageWidget(request)
+  ...     # In some versions, before rendering a widget, one has to set the name and id of the widget:
+  ...     image_widget.id = 'widget.id.image'
+  ...     image_widget.name = 'widget.name.image'
+  ...     return image_widget
+  >>> file_widget = make_file_widget(request)
+  >>> image_widget = make_image_widget(request)
 
 We also need to register the templates for the widgets::
 
@@ -89,24 +100,24 @@ We can extract simple file data from the widget like this::
   >>> import six
   >>> myfile = six.BytesIO(b'My file contents.')
 
-  >>> file_widget.request = make_request(form={'widget.name.file': myfile})
+  >>> file_widget = make_file_widget(make_request(form={'widget.name.file': myfile}))
   >>> file_widget.update()
   >>> file_widget.extract()
   <...IO object at ...>
 
-  >>> image_widget.request = make_request(form={'widget.name.image': myfile})
+  >>> image_widget = make_image_widget(make_request(form={'widget.name.image': myfile}))
   >>> image_widget.update()
   >>> image_widget.extract()
   <...IO object at ...>
 
 If nothing is found in the request, the default is returned::
 
-  >>> file_widget.request = make_request()
+  >>> file_widget = make_file_widget(make_request())
   >>> file_widget.update()
   >>> file_widget.extract()
   <NO_VALUE>
 
-  >>> image_widget.request = make_request()
+  >>> image_widget = make_image_widget(make_request())
   >>> image_widget.update()
   >>> image_widget.extract()
   <NO_VALUE>
@@ -130,17 +141,45 @@ Now build a FileUpload::
   >>> aFieldStorage = FieldStorageStub(myfile)
   >>> myUpload = FileUpload(aFieldStorage)
 
-  >>> file_widget.request = make_request(form={'widget.name.file': myUpload})
+First use a GET request::
+
+  >>> file_widget = make_file_widget(make_get_request(form={'widget.name.file': myUpload}))
   >>> file_widget.update()
   >>> file_widget.extract()
   <ZPublisher.HTTPRequest.FileUpload ...>
 
-  >>> image_widget.request = make_request(form={'widget.name.image': myUpload})
+  >>> image_widget = make_image_widget(make_get_request(form={'widget.name.image': myUpload}))
+  >>> image_widget.update()
+  >>> image_widget.extract()
+  <ZPublisher.HTTPRequest.FileUpload ...>
+
+The rendering is unchanged:
+
+  >>> print(file_widget.render())
+  <span id="widget.id.file" class="named-file-widget">
+      <input type="file" id="widget.id.file-input"
+             name="widget.name.file" />
+  </span>
+
+  >>> print(image_widget.render())
+  <span id="widget.id.image" class="named-image-widget">
+      <input type="file" id="widget.id.image-input"
+             name="widget.name.image" />
+  </span>
+
+Now use a POST request (the default in our make_request helper function)::
+
+  >>> file_widget = make_file_widget(make_request(form={'widget.name.file': myUpload}))
+  >>> file_widget.update()
+  >>> file_widget.extract()
+  <ZPublisher.HTTPRequest.FileUpload ...>
+
+  >>> image_widget = make_image_widget(make_request(form={'widget.name.image': myUpload}))
   >>> image_widget.update()
   >>> image_widget.extract()
   <ZPublisher.HTTPRequest.FileUpload ...>
 
-The rendering is unchanged::
+The rendering contains data about the file upload id::
 
   >>> print(file_widget.render())
   <span id="widget.id.file" class="named-file-widget">
@@ -170,12 +209,12 @@ Empty, unnamed FileUploads are treated as having no value::
   >>> aFieldStorage = FieldStorageStub(emptyfile, filename='')
   >>> myEmptyUpload = FileUpload(aFieldStorage)
 
-  >>> file_widget.request = make_request(form={'widget.name.file': myEmptyUpload})
+  >>> file_widget = make_file_widget(make_request(form={'widget.name.file': myEmptyUpload}))
   >>> file_widget.update()
   >>> file_widget.extract()
   <NO_VALUE>
 
-  >>> image_widget.request = make_request(form={'widget.name.image': myEmptyUpload})
+  >>> image_widget = make_image_widget(make_request(form={'widget.name.image': myEmptyUpload}))
   >>> image_widget.update()
   >>> image_widget.extract()
   <NO_VALUE>
@@ -456,7 +495,7 @@ content type::
   >>> file_obj = file_converter.toFieldValue(FileUpload(aFieldStorage))
   >>> file_obj.data
   b'File upload contents.'
-  >>> print(file_obj.filename.encode('utf8'))
+  >>> file_obj.filename.encode('utf8')
   b'rand\xc3\xb8m.txt'
 
 Content type from headers sent by browser should be ignored::
@@ -692,7 +731,7 @@ Check that we have a good image that PIL can handle::
 
   >>> content.image_field = bytes_image_converter.toFieldValue(uploaded)
   >>> content.image_field
-  b'filenameb64:aW1hZ2UuanBn;datab64:/9j/4AAQSkZJRgABAQEAYABgAAD/...
+  b'filenameb64:aW1hZ2UuanBn;datab64:/9j/4AAQSkZJRgABAQEAYABgAAD/...'
 
 Note that PIL cannot open this bytes image, so we cannot scale it::