Merge pull request #2564 from coreos-inc/userfiles-relative-test

Add fix for relative paths in user files lookup and add test

data/test/test_userfiles.py

@@ -0,0 +1,53 @@
+import pytest
+
+from mock import Mock
+from io import BytesIO
+
+from data.userfiles import DelegateUserfiles, Userfiles
+from test.fixtures import app, appconfig, database_uri, init_db_path, sqlitedb_file
+
+
+@pytest.mark.parametrize('prefix,path,expected', [
+  ('test', 'foo', 'test/foo'),
+  ('test', 'bar', 'test/bar'),
+  ('test', '/bar', 'test/bar'),
+  ('test', '../foo', 'test/foo'),
+  ('test', 'foo/bar/baz', 'test/baz'),
+  ('test', 'foo/../baz', 'test/baz'),
+
+  (None, 'foo', 'foo'),
+  (None, 'foo/bar/baz', 'baz'),
+])
+def test_filepath(prefix, path, expected):
+  userfiles = DelegateUserfiles(None, None, 'local_us', prefix)
+  assert userfiles.get_file_id_path(path) == expected
+
+
+def test_lookup_userfile(app, client):
+  uuid = 'deadbeef-dead-beef-dead-beefdeadbeef'
+  bad_uuid = 'deadduck-dead-duck-dead-duckdeadduck'
+  upper_uuid = 'DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF'
+
+  def _stream_read_file(locations, path):
+    if path.find(uuid) > 0 or path.find(upper_uuid) > 0:
+      return BytesIO("hello world")
+
+    raise IOError('Not found!')
+
+  storage_mock = Mock()
+  storage_mock.stream_read_file = _stream_read_file
+
+  app.config['USERFILES_PATH'] = 'foo'
+  Userfiles(app, distributed_storage=storage_mock)
+
+  rv = client.open('/userfiles/' + uuid, method='GET')
+  assert rv.status_code == 200
+
+  rv = client.open('/userfiles/' + upper_uuid, method='GET')
+  assert rv.status_code == 200
+
+  rv = client.open('/userfiles/' + bad_uuid, method='GET')
+  assert rv.status_code == 404
+
+  rv = client.open('/userfiles/foo/bar/baz', method='GET')
+  assert rv.status_code == 404

data/userfiles.py

@@ -32,6 +32,7 @@ class UserfilesHandlers(View):
       file_header_bytes = buffered.peek(1024)
       return send_file(buffered, mimetype=self._magic.from_buffer(file_header_bytes))
     except IOError:
+      logger.exception('Error reading user file')
       abort(404)
 
   def put(self, file_id):
@@ -73,7 +74,8 @@ class DelegateUserfiles(object):
                                   url_scheme=self._app.config['PREFERRED_URL_SCHEME'])
 
   def get_file_id_path(self, file_id):
-    return os.path.join(self._prefix, file_id)
+    # Note: We use basename here to prevent paths with ..'s and absolute paths.
+    return os.path.join(self._prefix or '', os.path.basename(file_id))
 
   def prepare_for_drop(self, mime_type, requires_cors=True):
     """ Returns a signed URL to upload a file to our bucket. """
@@ -135,12 +137,12 @@ class Userfiles(object):
     location = app.config.get('USERFILES_LOCATION')
     path = app.config.get('USERFILES_PATH', None)
 
+    if path is not None:
       handler_name = 'userfiles_handlers'
-
       userfiles = DelegateUserfiles(app, distributed_storage, location, path,
                                     handler_name=handler_name)
 
-    app.add_url_rule('/userfiles/<file_id>',
+      app.add_url_rule('/userfiles/<regex("[0-9a-zA-Z-]+"):file_id>',
                        view_func=UserfilesHandlers.as_view(handler_name,
                                                            distributed_storage=distributed_storage,
                                                            location=location,
@@ -149,6 +151,7 @@ class Userfiles(object):
       # register extension with app
       app.extensions = getattr(app, 'extensions', {})
       app.extensions['userfiles'] = userfiles
+
     return userfiles
 
   def __getattr__(self, name):