TempURL: Fixed bug with \r or \n in disposition.

If an object had a \r or \n in its name, it would end up creating an
invalid HTTP Content-Disposition header. Reviewer consensus was to use
URL encoding.

Fixes bug 1306250

Change-Id: Ibccaaed5152b4d09d6aee4966a1982cc0a0da07d

2 files changed, 58 insertions, 8 deletions

diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py
index 57486941b..61af1acfb 100644
--- a/swift/common/middleware/tempurl.py
+++ b/swift/common/middleware/tempurl.py
@@ -1,4 +1,19 @@
-# Copyright (c) 2010-2012 OpenStack Foundation
+# Copyright (c) 2011-2014 Greg Holt
+# Copyright (c) 2012-2013 John Dickinson
+# Copyright (c) 2012 Felipe Reyes
+# Copyright (c) 2012 Peter Portante
+# Copyright (c) 2012 Victor Rodionov
+# Copyright (c) 2013-2014 Samuel Merritt
+# Copyright (c) 2013 Chuck Thier
+# Copyright (c) 2013 David Goetz
+# Copyright (c) 2013 Dirk Mueller
+# Copyright (c) 2013 Donagh McCabe
+# Copyright (c) 2013 Fabien Boucher
+# Copyright (c) 2013 Greg Lange
+# Copyright (c) 2013 Kun Huang
+# Copyright (c) 2013 Richard Hawkins
+# Copyright (c) 2013 Tong Li
+# Copyright (c) 2013 ZhiQiang Fan
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -106,7 +121,7 @@ from urlparse import parse_qs
 from swift.proxy.controllers.base import get_account_info
 from swift.common.swob import HeaderKeyDict, HTTPUnauthorized
 from swift.common.utils import split_path, get_valid_utf8_str, \
-    register_swift_info, get_hmac, streq_const_time
+    register_swift_info, get_hmac, streq_const_time, quote
 
 
 #: Default headers to remove from incoming requests. Simply a whitespace
@@ -147,6 +162,10 @@ def get_tempurl_keys_from_metadata(meta):
             if key.lower() in ('temp-url-key', 'temp-url-key-2')]
 
 
+def disposition_format(filename):
+    return 'attachment; filename="%s"' % quote(filename, safe='/ ')
+
+
 class TempURL(object):
     """
     WSGI Middleware to grant temporary URLs specific access to Swift
@@ -319,14 +338,12 @@ class TempURL(object):
                 if inline_disposition:
                     disposition_value = 'inline'
                 elif filename:
-                    disposition_value = 'attachment; filename="%s"' % (
-                        filename.replace('"', '\\"'))
+                    disposition_value = disposition_format(filename)
                 elif existing_disposition:
                     disposition_value = existing_disposition
                 else:
                     name = basename(env['PATH_INFO'].rstrip('/'))
-                    disposition_value = 'attachment; filename="%s"' % (
-                        name.replace('"', '\\"'))
+                    disposition_value = disposition_format(name)
                 out_headers.append(('Content-Disposition', disposition_value))
                 headers = out_headers
             return start_response(status, headers, exc_info)
diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py
index 157186aef..f94043417 100644
--- a/test/unit/common/middleware/test_tempurl.py
+++ b/test/unit/common/middleware/test_tempurl.py
@@ -1,4 +1,19 @@
-# Copyright (c) 2011 OpenStack Foundation
+# Copyright (c) 2011-2014 Greg Holt
+# Copyright (c) 2012-2013 Peter Portante
+# Copyright (c) 2012 Iryoung Jeong
+# Copyright (c) 2012 Michael Barton
+# Copyright (c) 2013 Alex Gaynor
+# Copyright (c) 2013 Chuck Thier
+# Copyright (c) 2013 David Goetz
+# Copyright (c) 2013 Donagh McCabe
+# Copyright (c) 2013 Greg Lange
+# Copyright (c) 2013 John Dickinson
+# Copyright (c) 2013 Kun Huang
+# Copyright (c) 2013 Richard Hawkins
+# Copyright (c) 2013 Samuel Merritt
+# Copyright (c) 2013 Shri Javadekar
+# Copyright (c) 2013 Tong Li
+# Copyright (c) 2013 ZhiQiang Fan
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -142,7 +157,7 @@ class TestTempURL(unittest.TestCase):
         resp = req.get_response(self.tempurl)
         self.assertEquals(resp.status_int, 200)
         self.assertEquals(resp.headers['content-disposition'],
-                          'attachment; filename="bob \\\"killer\\\".txt"')
+                          'attachment; filename="bob %22killer%22.txt"')
         self.assertEquals(req.environ['swift.authorize_override'], True)
         self.assertEquals(req.environ['REMOTE_USER'], '.wsgi.tempurl')
 
@@ -195,6 +210,24 @@ class TestTempURL(unittest.TestCase):
         self.assertEquals(req.environ['swift.authorize_override'], True)
         self.assertEquals(req.environ['REMOTE_USER'], '.wsgi.tempurl')
 
+    def test_obj_odd_chars(self):
+        method = 'GET'
+        expires = int(time() + 86400)
+        path = '/v1/a/c/a\r\nb'
+        key = 'abc'
+        hmac_body = '%s\n%s\n%s' % (method, expires, path)
+        sig = hmac.new(key, hmac_body, sha1).hexdigest()
+        req = self._make_request(path, keys=[key], environ={
+            'QUERY_STRING': 'temp_url_sig=%s&temp_url_expires=%s' % (
+                sig, expires)})
+        self.tempurl.app = FakeApp(iter([('200 Ok', (), '123')]))
+        resp = req.get_response(self.tempurl)
+        self.assertEquals(resp.status_int, 200)
+        self.assertEquals(resp.headers['content-disposition'],
+                          'attachment; filename="a%0D%0Ab"')
+        self.assertEquals(req.environ['swift.authorize_override'], True)
+        self.assertEquals(req.environ['REMOTE_USER'], '.wsgi.tempurl')
+
     def test_obj_trailing_slash(self):
         method = 'GET'
         expires = int(time() + 86400)