Properly remove f.name usage in send_file (#1988)

* Properly remove f.name usage in send_file

* Update changelogs

* Fix tests

CHANGES

@@ -7,8 +7,10 @@ Version 0.12
 ------------
 
 - the cli command now responds to `--version`.
-- Mimetype guessing for ``send_file`` has been removed, as per issue ``#104``.
-  See pull request ``#1849``.
+- Mimetype guessing and ETag generation for file-like objects in ``send_file``
+  has been removed, as per issue ``#104``.  See pull request ``#1849``.
+- Mimetype guessing in ``send_file`` now fails loudly and doesn't fall back to
+  ``application/octet-stream``. See pull request ``#1988``.
 - Make ``flask.safe_join`` able to join multiple paths like ``os.path.join``
   (pull request ``#1730``).
 

docs/upgrading.rst

@@ -19,7 +19,45 @@ providing the ``--upgrade`` parameter::
 
     $ pip install --upgrade Flask
 
-.. _upgrading-to-10:
+.. _upgrading-to-012:
+
+Version 0.12
+------------
+
+Changes to send_file
+````````````````````
+
+The ``filename`` is no longer automatically inferred from file-like objects.
+This means that the following code will no longer automatically have
+``X-Sendfile`` support, etag generation or MIME-type guessing::
+
+    response = send_file(open('/path/to/file.txt'))
+
+Any of the following is functionally equivalent::
+
+    fname = '/path/to/file.txt'
+
+    # Just pass the filepath directly
+    response = send_file(fname)
+
+    # Set the MIME-type and ETag explicitly
+    response = send_file(open(fname), mimetype='text/plain')
+    response.set_etag(...)
+
+    # Set `attachment_filename` for MIME-type guessing
+    # ETag still needs to be manually set
+    response = send_file(open(fname), attachment_filename=fname)
+    response.set_etag(...)
+
+The reason for this is that some file-like objects have a invalid or even
+misleading ``name`` attribute. Silently swallowing errors in such cases was not
+a satisfying solution.
+
+Additionally the default of falling back to ``application/octet-stream`` has
+been removed. If Flask can't guess one or the user didn't provide one, the
+function fails.
+
+.. _upgrading-to-011:
 
 Version 0.11
 ------------

flask/helpers.py

@@ -437,7 +437,14 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
     to ``True`` to directly emit an ``X-Sendfile`` header.  This however
     requires support of the underlying webserver for ``X-Sendfile``.
 
-    You must explicitly provide the mimetype for the filename or file object.
+    By default it will try to guess the mimetype for you, but you can
+    also explicitly provide one.  For extra security you probably want
+    to send certain files as attachment (HTML for instance).  The mimetype
+    guessing requires a `filename` or an `attachment_filename` to be
+    provided.
+
+    ETags will also be attached automatically if a `filename` is provided. You
+    can turn this off by setting `add_etags=False`.
 
     Please never pass filenames to this function from user sources;
     you should use :func:`send_from_directory` instead.
@@ -458,9 +465,13 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
        cache_timeout pulls its default from application config, when None.
 
     .. versionchanged:: 0.12
-       mimetype guessing and etag support removed for file objects.
-       If no mimetype or attachment_filename is provided, application/octet-stream
-       will be used.
+       The filename is no longer automatically inferred from file objects. If
+       you want to use automatic mimetype and etag support, pass a filepath via
+       `filename_or_fp` or `attachment_filename`.
+
+    .. versionchanged:: 0.12
+       The `attachment_filename` is preferred over `filename` for MIME-type
+       detection.
 
     :param filename_or_fp: the filename of the file to send in `latin-1`.
                            This is relative to the :attr:`~Flask.root_path`
@@ -470,8 +481,9 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
                            back to the traditional method.  Make sure that the
                            file pointer is positioned at the start of data to
                            send before calling :func:`send_file`.
-    :param mimetype: the mimetype of the file if provided, otherwise
-                     auto detection happens.
+    :param mimetype: the mimetype of the file if provided. If a file path is
+                     given, auto detection happens as fallback, otherwise an
+                     error will be raised.
     :param as_attachment: set to ``True`` if you want to send this file with
                           a ``Content-Disposition: attachment`` header.
     :param attachment_filename: the filename for the attachment if it
@@ -490,26 +502,36 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
     mtime = None
     if isinstance(filename_or_fp, string_types):
         filename = filename_or_fp
+        if not os.path.isabs(filename):
+            filename = os.path.join(current_app.root_path, filename)
         file = None
+        if attachment_filename is None:
+            attachment_filename = os.path.basename(filename)
     else:
         file = filename_or_fp
-        filename = getattr(file, 'name', None)
+        filename = None
 
-    if filename is not None:
-        if not os.path.isabs(filename):
-            filename = os.path.join(current_app.root_path, filename)
-    if mimetype is None and (filename or attachment_filename):
-        mimetype = mimetypes.guess_type(filename or attachment_filename)[0]
     if mimetype is None:
-        mimetype = 'application/octet-stream'
+        if attachment_filename is not None:
+            mimetype = mimetypes.guess_type(attachment_filename)[0]
+
+        if mimetype is None:
+            if attachment_filename is not None:
+                raise ValueError(
+                    'Unable to infer MIME-type from filename {!r}, please '
+                    'pass one explicitly.'.format(mimetype_filename)
+                )
+            raise ValueError(
+                'Unable to infer MIME-type because no filename is available. '
+                'Please set either `attachment_filename`, pass a filepath to '
+                '`filename_or_fp` or set your own MIME-type via `mimetype`.'
+            )
 
     headers = Headers()
     if as_attachment:
         if attachment_filename is None:
-            if filename is None:
             raise TypeError('filename unavailable, required for '
                             'sending as attachment')
-            attachment_filename = os.path.basename(filename)
         headers.add('Content-Disposition', 'attachment',
                     filename=attachment_filename)
 

tests/test_helpers.py

@@ -384,18 +384,30 @@ class TestSendfile(object):
 
         @app.route('/')
         def index():
-            return flask.send_file(StringIO("party like it's"), last_modified=last_modified)
+            return flask.send_file(StringIO("party like it's"),
+                                   last_modified=last_modified,
+                                   mimetype='text/plain')
 
         c = app.test_client()
         rv = c.get('/')
         assert rv.last_modified == last_modified
 
+    def test_send_file_object_without_mimetype(self):
+        app = flask.Flask(__name__)
+
+        with app.test_request_context():
+            with pytest.raises(ValueError) as excinfo:
+                flask.send_file(StringIO("LOL"))
+
+            assert 'Unable to infer MIME-type' in str(excinfo)
+            assert 'no filename is available' in str(excinfo)
+
     def test_send_file_object(self):
         app = flask.Flask(__name__)
 
         with app.test_request_context():
             with open(os.path.join(app.root_path, 'static/index.html'), mode='rb') as f:
-                rv = flask.send_file(f)
+                rv = flask.send_file(f, mimetype='text/html')
                 rv.direct_passthrough = False
                 with app.open_resource('static/index.html') as f:
                     assert rv.data == f.read()
@@ -406,17 +418,15 @@ class TestSendfile(object):
 
         with app.test_request_context():
             with open(os.path.join(app.root_path, 'static/index.html')) as f:
-                rv = flask.send_file(f)
+                rv = flask.send_file(f, mimetype='text/html')
                 assert rv.mimetype == 'text/html'
-                assert 'x-sendfile' in rv.headers
-                assert rv.headers['x-sendfile'] == \
-                    os.path.join(app.root_path, 'static/index.html')
+                assert 'x-sendfile' not in rv.headers
                 rv.close()
 
         app.use_x_sendfile = False
         with app.test_request_context():
             f = StringIO('Test')
-            rv = flask.send_file(f)
+            rv = flask.send_file(f, mimetype='application/octet-stream')
             rv.direct_passthrough = False
             assert rv.data == b'Test'
             assert rv.mimetype == 'application/octet-stream'
@@ -429,7 +439,7 @@ class TestSendfile(object):
                     return getattr(self._io, name)
             f = PyStringIO('Test')
             f.name = 'test.txt'
-            rv = flask.send_file(f)
+            rv = flask.send_file(f, attachment_filename=f.name)
             rv.direct_passthrough = False
             assert rv.data == b'Test'
             assert rv.mimetype == 'text/plain'
@@ -446,7 +456,7 @@ class TestSendfile(object):
 
         with app.test_request_context():
             f = StringIO('Test')
-            rv = flask.send_file(f)
+            rv = flask.send_file(f, mimetype='text/html')
             assert 'x-sendfile' not in rv.headers
             rv.close()
 
@@ -454,7 +464,8 @@ class TestSendfile(object):
         app = flask.Flask(__name__)
         with app.test_request_context():
             with open(os.path.join(app.root_path, 'static/index.html')) as f:
-                rv = flask.send_file(f, as_attachment=True)
+                rv = flask.send_file(f, as_attachment=True,
+                                     attachment_filename='index.html')
                 value, options = \
                     parse_options_header(rv.headers['Content-Disposition'])
                 assert value == 'attachment'