From 0fd1853b95bf3d79253f771053ddeb347ac2563b Mon Sep 17 00:00:00 2001 From: Joe Gregorio Date: Fri, 24 Aug 2012 15:54:40 -0400 Subject: [PATCH] Do not allow credentials files to be symlinks. Reviewed in https://codereview.appspot.com/6476062/. --- oauth2client/file.py | 17 +++++++++++++ oauth2client/locked_file.py | 15 +++++++++++ oauth2client/multistore_file.py | 2 +- tests/test_oauth2client_file.py | 44 +++++++++++++++++++++++++++------ 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/oauth2client/file.py b/oauth2client/file.py index 1abc6d2..1895f94 100644 --- a/oauth2client/file.py +++ b/oauth2client/file.py @@ -29,6 +29,10 @@ from client import Storage as BaseStorage from client import Credentials +class CredentialsFileSymbolicLinkError(Exception): + """Credentials files must not be symbolic links.""" + + class Storage(BaseStorage): """Store and retrieve a single credential to and from a file.""" @@ -36,6 +40,11 @@ class Storage(BaseStorage): self._filename = filename self._lock = threading.Lock() + def _validate_file(self): + if os.path.islink(self._filename): + raise CredentialsFileSymbolicLinkError( + 'File: %s is a symbolic link.' % self._filename) + def acquire_lock(self): """Acquires any lock necessary to access this Storage. @@ -55,8 +64,12 @@ class Storage(BaseStorage): Returns: oauth2client.client.Credentials + + Raises: + CredentialsFileSymbolicLinkError if the file is a symbolic link. """ credentials = None + self._validate_file() try: f = open(self._filename, 'rb') content = f.read() @@ -90,9 +103,13 @@ class Storage(BaseStorage): Args: credentials: Credentials, the credentials to store. + + Raises: + CredentialsFileSymbolicLinkError if the file is a symbolic link. """ self._create_file_if_needed() + self._validate_file() f = open(self._filename, 'wb') f.write(credentials.to_json()) f.close() diff --git a/oauth2client/locked_file.py b/oauth2client/locked_file.py index 8f35c90..1cfe532 100644 --- a/oauth2client/locked_file.py +++ b/oauth2client/locked_file.py @@ -28,11 +28,20 @@ from oauth2client import util logger = logging.getLogger(__name__) +class CredentialsFileSymbolicLinkError(Exception): + """Credentials files must not be symbolic links.""" + + class AlreadyLockedException(Exception): """Trying to lock a file that has already been locked by the LockedFile.""" pass +def validate_file(filename): + if os.path.islink(filename): + raise CredentialsFileSymbolicLinkError( + 'File: %s is a symbolic link.' % filename) + class _Opener(object): """Base class for different locking primitives.""" @@ -91,12 +100,14 @@ class _PosixOpener(_Opener): Raises: AlreadyLockedException: if the lock is already acquired. IOError: if the open fails. + CredentialsFileSymbolicLinkError if the file is a symbolic link. """ if self._locked: raise AlreadyLockedException('File %s is already locked' % self._filename) self._locked = False + validate_file(self._filename) try: self._fh = open(self._filename, self._mode) except IOError, e: @@ -159,12 +170,14 @@ try: Raises: AlreadyLockedException: if the lock is already acquired. IOError: if the open fails. + CredentialsFileSymbolicLinkError if the file is a symbolic link. """ if self._locked: raise AlreadyLockedException('File %s is already locked' % self._filename) start_time = time.time() + validate_file(self._filename) try: self._fh = open(self._filename, self._mode) except IOError, e: @@ -232,12 +245,14 @@ try: Raises: AlreadyLockedException: if the lock is already acquired. IOError: if the open fails. + CredentialsFileSymbolicLinkError if the file is a symbolic link. """ if self._locked: raise AlreadyLockedException('File %s is already locked' % self._filename) start_time = time.time() + validate_file(self._filename) try: self._fh = open(self._filename, self._mode) except IOError, e: diff --git a/oauth2client/multistore_file.py b/oauth2client/multistore_file.py index e190c6a..c919573 100644 --- a/oauth2client/multistore_file.py +++ b/oauth2client/multistore_file.py @@ -76,7 +76,7 @@ def get_credential_storage(filename, client_id, user_agent, scope, An object derived from client.Storage for getting/setting the credential. """ - filename = os.path.realpath(os.path.expanduser(filename)) + filename = os.path.expanduser(filename) _multistores_lock.acquire() try: multistore = _multistores.setdefault( diff --git a/tests/test_oauth2client_file.py b/tests/test_oauth2client_file.py index 596a4b6..7c65677 100644 --- a/tests/test_oauth2client_file.py +++ b/tests/test_oauth2client_file.py @@ -32,12 +32,13 @@ import tempfile import unittest from apiclient.http import HttpMockSequence +from oauth2client import file +from oauth2client import locked_file from oauth2client import multistore_file from oauth2client.anyjson import simplejson from oauth2client.client import AccessTokenCredentials from oauth2client.client import AssertionCredentials from oauth2client.client import OAuth2Credentials -from oauth2client.file import Storage FILENAME = tempfile.mktemp('oauth2client_test.data') @@ -58,10 +59,23 @@ class OAuth2ClientFileTests(unittest.TestCase): pass def test_non_existent_file_storage(self): - s = Storage(FILENAME) + s = file.Storage(FILENAME) credentials = s.get() self.assertEquals(None, credentials) + def test_no_sym_link_credentials(self): + if hasattr(os, 'symlink'): + SYMFILENAME = FILENAME + '.sym' + os.symlink(FILENAME, SYMFILENAME) + s = file.Storage(SYMFILENAME) + try: + s.get() + self.fail('Should have raised an exception.') + except file.CredentialsFileSymbolicLinkError: + pass + finally: + os.unlink(SYMFILENAME) + def test_pickle_and_json_interop(self): # Write a file with a pickled OAuth2Credentials. access_token = 'foo' @@ -83,13 +97,13 @@ class OAuth2ClientFileTests(unittest.TestCase): # Storage should be not be able to read that object, as the capability to # read and write credentials as pickled objects has been removed. - s = Storage(FILENAME) + s = file.Storage(FILENAME) read_credentials = s.get() self.assertEquals(None, read_credentials) # Now write it back out and confirm it has been rewritten as JSON s.put(credentials) - f = file(FILENAME) + f = open(FILENAME) data = simplejson.load(f) f.close() @@ -111,7 +125,7 @@ class OAuth2ClientFileTests(unittest.TestCase): refresh_token, token_expiry, token_uri, user_agent) - s = Storage(FILENAME) + s = file.Storage(FILENAME) s.put(credentials) credentials = s.get() new_cred = copy.copy(credentials) @@ -135,7 +149,7 @@ class OAuth2ClientFileTests(unittest.TestCase): refresh_token, token_expiry, token_uri, user_agent) - s = Storage(FILENAME) + s = file.Storage(FILENAME) s.put(credentials) credentials = s.get() self.assertNotEquals(None, credentials) @@ -149,7 +163,7 @@ class OAuth2ClientFileTests(unittest.TestCase): credentials = AccessTokenCredentials(access_token, user_agent) - s = Storage(FILENAME) + s = file.Storage(FILENAME) credentials = s.put(credentials) credentials = s.get() @@ -188,6 +202,22 @@ class OAuth2ClientFileTests(unittest.TestCase): self.assertTrue(store._multistore._read_only) os.chmod(FILENAME, 0600) + def test_multistore_no_symbolic_link_files(self): + if hasattr(os, 'symlink'): + SYMFILENAME = FILENAME + 'sym' + os.symlink(FILENAME, SYMFILENAME) + store = multistore_file.get_credential_storage( + SYMFILENAME, + 'some_client_id', + 'user-agent/1.0', + ['some-scope', 'some-other-scope']) + try: + store.get() + self.fail('Should have raised an exception.') + except locked_file.CredentialsFileSymbolicLinkError: + pass + finally: + os.unlink(SYMFILENAME) def test_multistore_non_existent_file(self): store = multistore_file.get_credential_storage(