feat: Implement directory validation and shell command quoting

- Added is_valid_directory function to validate directory paths, ensuring they do not contain control characters and are within a specified length.
- Introduced quote_shell_arg to safely quote shell arguments, preventing command injection.
- Created build_cd_command to generate a command for changing directories in a shell.
- Enhanced the LoginHandler to utilize a login rate limiter, preventing brute-force attacks by tracking failed login attempts.
- Implemented an EncodingCache to optimize encoding detection for SSH connections.
- Updated the UI to include an input field for specifying an initial directory upon login, with appropriate validation and hints.
- Added a quickbar in the terminal interface for easy access to copy and paste functionality.
- Introduced a toast notification system to provide feedback on copy actions.
- Refactored connection storage to encrypt passwords at rest, improving security.
- Updated various templates and styles to accommodate new features and improve user experience.

tests/sshserver.py

@@ -20,6 +20,9 @@ print('Read key: ' + hexlify(host_key.get_fingerprint()).decode('utf-8'))
 
 banner = u'\r\n\u6b22\u8fce\r\n'
 event_timeout = 5
+# clients that reuse a cached encoding never send the probe command, so this
+# wait must be short and non-fatal
+exec_timeout = 1
 
 
 class Server(paramiko.ServerInterface):
@@ -41,6 +44,7 @@ class Server(paramiko.ServerInterface):
         self.shell_event = threading.Event()
         self.exec_event = threading.Event()
         self.cmd_to_enc = self.get_cmd2enc(encodings)
+        self.encoding = 'UTF-8'
         self.password_verified = False
         self.key_verified = False
 
@@ -171,10 +175,11 @@ def run_ssh_server(port=2200, running=True, encodings=[]):
             print('*** Client never asked for a shell.')
             continue
 
-        server.exec_event.wait(timeout=event_timeout)
+        server.exec_event.wait(timeout=exec_timeout)
         if not server.exec_event.is_set():
+            # the client already knew this server's encoding and skipped the
+            # probe; fall back to the default instead of dropping the session
             print('*** Client never asked for a command.')
-            continue
 
         # chan.send('\r\n\r\nWelcome!\r\n\r\n')
         print(server.encoding)

tests/test_app.py

@@ -1,5 +1,7 @@
 import json
 import random
+import shutil
+import tempfile
 import threading
 import tornado.websocket
 import tornado.gen
@@ -29,6 +31,18 @@ server_encodings = {e.strip() for e in Server.encodings}
 
 class TestAppBase(AsyncHTTPTestCase):
 
+    def setUp(self):
+        # keep auth.json/connections.json/cookie_secret out of the real
+        # data directory while testing
+        self.tmpdir = tempfile.mkdtemp()
+        options.data_dir = self.tmpdir
+        handler.encoding_cache.clear()
+        super(TestAppBase, self).setUp()
+
+    def tearDown(self):
+        super(TestAppBase, self).tearDown()
+        shutil.rmtree(self.tmpdir, ignore_errors=True)
+
     def get_httpserver_options(self):
         return get_server_settings(options)
 
@@ -327,6 +341,48 @@ class TestAppBasic(TestAppBase):
         self.assertEqual(b'bye', msg)
         ws.close()
 
+    def test_app_with_invalid_directory(self):
+        body = urlencode({
+            'hostname': '127.0.0.1',
+            'port': str(self.sshserver_port),
+            '_xsrf': 'yummy',
+            'username': 'robey',
+            'password': 'foo',
+            'directory': '/tmp\nreboot'
+        })
+        response = self.sync_post('/', body)
+        self.assert_response(b'Invalid directory', response)
+
+    @tornado.testing.gen_test
+    def test_app_with_directory_runs_cd_after_login(self):
+        body = urlencode({
+            'hostname': '127.0.0.1',
+            'port': str(self.sshserver_port),
+            '_xsrf': 'yummy',
+            'username': 'bar',
+            'password': 'foo',
+            'directory': "/var/o'brien"
+        })
+        url = self.get_url('/')
+        response = yield self.async_post(url, body)
+        data = json.loads(to_str(response.body))
+        self.assert_status_none(data)
+
+        url = url.replace('http', 'ws')
+        ws_url = url + 'ws?id=' + data['id']
+        ws = yield tornado.websocket.websocket_connect(ws_url)
+
+        # the mock server echoes back whatever the shell channel receives;
+        # the banner and the echo may or may not arrive in the same frame
+        received = b''
+        for _ in range(3):
+            received += (yield ws.read_message())
+            if b'cd ' in received:
+                break
+
+        self.assertIn(b"cd '/var/o'\\''brien'\r", received)
+        ws.close()
+
     @tornado.testing.gen_test
     def test_app_auth_with_valid_pubkey_by_urlencoded_form(self):
         url = self.get_url('/')
@@ -792,3 +848,57 @@ class TestAppWithUnknownEncoding(OtherTestBase):
         dic = json.loads(to_str(response.body))
         self.assert_status_none(dic)
         self.assertEqual(dic['encoding'], 'utf-8')
+
+
+class TestAppWithSavedConnections(OtherTestBase):
+
+    def async_get(self, path):
+        return self.get_http_client().fetch(
+            self.get_url(path), headers=self.headers
+        )
+
+    @tornado.testing.gen_test
+    def test_saved_password_never_reaches_the_browser(self):
+        body = dict(self.body, save='1')
+        response = yield self.async_post('/', body)
+        data = json.loads(to_str(response.body))
+        self.assert_status_none(data)
+
+        profile = data['profile']
+        self.assertNotIn('password', profile)
+        self.assertNotIn('password_enc', profile)
+        self.assertTrue(profile['has_password'])
+
+        response = yield self.async_get('/connections')
+        self.assertNotIn(b'"foo"', response.body)
+        self.assertNotIn(b'password_enc', response.body)
+
+        listed = json.loads(to_str(response.body))['connections']
+        self.assertEqual(1, len(listed))
+        self.assertNotIn('password', listed[0])
+        self.assertTrue(listed[0]['has_password'])
+
+    @tornado.testing.gen_test
+    def test_saved_password_is_reused_when_the_field_is_left_empty(self):
+        body = dict(self.body, save='1')
+        response = yield self.async_post('/', body)
+        self.assert_status_none(json.loads(to_str(response.body)))
+
+        # no password in the form at all: the server supplies the stored one
+        body = dict(self.body, password='')
+        response = yield self.async_post('/', body)
+        self.assert_status_none(json.loads(to_str(response.body)))
+
+    @tornado.testing.gen_test
+    def test_saved_password_is_not_sent_to_another_destination(self):
+        body = dict(self.body, save='1')
+        response = yield self.async_post('/', body)
+        self.assert_status_none(json.loads(to_str(response.body)))
+
+        # a different username means a different profile id, so the stored
+        # credential must not be reused here
+        body = dict(self.body, username='bar', password='')
+        response = yield self.async_post('/', body)
+        self.assert_status_equal(
+            'Authentication failed.', json.loads(to_str(response.body))
+        )

tests/test_auth_app.py

@@ -5,6 +5,8 @@ import tempfile
 from tornado.options import options
 from tornado.testing import AsyncHTTPTestCase
 
+from webssh.handler import login_rate_limiter
+from webssh.worker import clients
 from webssh.main import make_app, make_handlers
 from webssh.settings import get_app_settings
 from webssh.utils import to_str
@@ -62,3 +64,101 @@ class TestAppAuth(AsyncHTTPTestCase):
         response = self.fetch('/connections', headers=headers)
         data = json.loads(to_str(response.body))
         self.assertEqual(data, {'connections': []})
+
+    def test_login_is_rate_limited_after_repeated_failures(self):
+        login_rate_limiter.clear()
+        self.addCleanup(login_rate_limiter.clear)
+
+        body = 'username=admin&password=secret&confirm=secret'
+        response = self.fetch('/login', method='POST', body=body,
+                              follow_redirects=False)
+        self.assertEqual(response.code, 302)
+
+        wrong = 'username=admin&password=nope'
+        for _ in range(login_rate_limiter.max_attempts):
+            response = self.fetch('/login', method='POST', body=wrong)
+            self.assertIn('用户名或密码错误'.encode('utf-8'), response.body)
+
+        # further attempts are refused without touching the password hash
+        response = self.fetch('/login', method='POST', body=wrong)
+        self.assertIn('登录尝试过于频繁'.encode('utf-8'), response.body)
+
+        # ... and a correct password is refused too while the block lasts
+        good = 'username=admin&password=secret'
+        response = self.fetch('/login', method='POST', body=good,
+                              follow_redirects=False)
+        self.assertEqual(response.code, 200)
+        self.assertIn('登录尝试过于频繁'.encode('utf-8'), response.body)
+
+    def test_successful_login_clears_the_failure_counter(self):
+        login_rate_limiter.clear()
+        self.addCleanup(login_rate_limiter.clear)
+
+        body = 'username=admin&password=secret&confirm=secret'
+        self.fetch('/login', method='POST', body=body, follow_redirects=False)
+
+        self.fetch('/login', method='POST', body='username=admin&password=x')
+        self.assertNotEqual({}, login_rate_limiter.attempts)
+
+        response = self.fetch('/login', method='POST',
+                              body='username=admin&password=secret',
+                              follow_redirects=False)
+        self.assertEqual(response.code, 302)
+        self.assertEqual({}, login_rate_limiter.attempts)
+
+
+class TestMaxConnPerUser(AsyncHTTPTestCase):
+
+    def setUp(self):
+        self.tmpdir = tempfile.mkdtemp()
+        clients.clear()
+        login_rate_limiter.clear()
+        self.addCleanup(clients.clear)
+        self.addCleanup(login_rate_limiter.clear)
+        super(TestMaxConnPerUser, self).setUp()
+
+    def tearDown(self):
+        super(TestMaxConnPerUser, self).tearDown()
+        shutil.rmtree(self.tmpdir, ignore_errors=True)
+
+    def get_app(self):
+        loop = self.io_loop
+        options.auth = True
+        options.auth_username = 'admin'
+        options.auth_password = 'secret'
+        options.auth_password_hash = ''
+        options.auth_session_days = 7
+        options.data_dir = self.tmpdir
+        options.debug = False
+        options.xsrf = False
+        options.policy = 'warning'
+        options.hostfile = ''
+        options.syshostfile = ''
+        options.tdstream = ''
+        options.origin = 'same'
+        options.maxconn = 1
+        return make_app(make_handlers(loop, options), get_app_settings(options))
+
+    def login(self):
+        response = self.fetch('/login', method='POST',
+                              body='username=admin&password=secret',
+                              follow_redirects=False)
+        cookie = response.headers.get_list('Set-Cookie')[0]
+        return {'Cookie': cookie.split(';', 1)[0]}
+
+    def test_limit_follows_the_login_not_the_address(self):
+        headers = self.login()
+        body = 'hostname=127.0.0.1&port=65000&username=robey'
+
+        # one live session for this login already exhausts maxconn=1
+        clients['user:admin'] = {'fake': None}
+        response = self.fetch('/', method='POST', body=body, headers=headers)
+        self.assertIn(b'Too many live connections', response.body)
+
+        # another user shares the same source address but has its own budget,
+        # so it gets as far as actually dialing the (absent) ssh server
+        clients.clear()
+        clients['user:someone-else'] = {'fake': None}
+        response = self.fetch('/', method='POST', body=body, headers=headers)
+        self.assertNotIn(b'Too many live connections', response.body)
+        self.assertIn(b'Unable to connect to', response.body)

tests/test_handler.py

@@ -8,7 +8,7 @@ from tests.utils import read_file, make_tests_data_path
 from webssh import handler
 from webssh import worker
 from webssh.handler import (
-    IndexHandler, MixinHandler, WsockHandler, PrivateKey, InvalidValueError, SSHClient
+    IndexHandler, MixinHandler, WsockHandler, PrivateKey, InvalidValueError, SSHClient, LoginRateLimiter, EncodingCache
 )
 
 try:
@@ -324,6 +324,7 @@ class TestIndexHandler(unittest.TestCase):
         obj.get_port.return_value = 22
         obj.get_value.return_value = 'root'
         obj.get_privatekey.return_value = ('', '')
+        obj.get_directory.return_value = ''
         obj.policy = paramiko.WarningPolicy()
         obj.ssh_client = Mock()
 
@@ -345,16 +346,75 @@ class TestIndexHandler(unittest.TestCase):
         )
         self.assertEqual('root-secret', obj.connection_info['password'])
 
+    def test_get_args_carries_directory(self):
+        obj = Mock(spec=IndexHandler)
+        obj.get_hostname.return_value = '127.0.0.1'
+        obj.get_port.return_value = 22
+        obj.get_value.return_value = 'root'
+        obj.get_privatekey.return_value = ('', '')
+        obj.get_directory.return_value = '/var/www'
+        obj.policy = paramiko.WarningPolicy()
+        obj.ssh_client = Mock()
+        obj.get_argument.side_effect = lambda name, default=u'': default
+
+        args = IndexHandler.get_args(obj)
+
+        self.assertEqual('/var/www', args[6])
+        self.assertEqual('/var/www', obj.connection_info['directory'])
+
+    def test_get_directory(self):
+        obj = Mock(spec=IndexHandler)
+        values = {}
+        obj.get_argument.side_effect = lambda name, default=u'': values.get(
+            name, default
+        )
+
+        self.assertEqual('', IndexHandler.get_directory(obj))
+
+        values['directory'] = '   '
+        self.assertEqual('', IndexHandler.get_directory(obj))
+
+        values['directory'] = '  /var/www  '
+        self.assertEqual('/var/www', IndexHandler.get_directory(obj))
+
+        values['directory'] = '/tmp\nrm -rf /'
+        with self.assertRaises(InvalidValueError):
+            IndexHandler.get_directory(obj)
+
+        values['directory'] = '/tmp' + 'a' * 1024
+        with self.assertRaises(InvalidValueError):
+            IndexHandler.get_directory(obj)
+
+    def test_change_directory_quotes_the_path(self):
+        obj = Mock(spec=IndexHandler)
+        chan = Mock()
+
+        IndexHandler.change_directory(obj, chan, '/tmp; reboot')
+
+        chan.sendall.assert_called_with(b"cd '/tmp; reboot'\r")
+
+    def test_change_directory_swallows_channel_errors(self):
+        obj = Mock(spec=IndexHandler)
+        chan = Mock()
+        chan.sendall.side_effect = paramiko.SSHException('boom')
+
+        # a failed cd must not prevent the session from starting
+        IndexHandler.change_directory(obj, chan, '/var/www')
+
     def test_null_in_encoding(self):
-        handler = Mock(spec=IndexHandler)
+        mock_handler = Mock(spec=IndexHandler)
 
         # This is a little nasty, but the index handler has a lot of
         # dependencies to mock. Mocking out everything but the bits
         # we want to test lets us test this case without needing to
         # refactor the relevant code out of IndexHandler
         def parse_encoding(data):
-            return IndexHandler.parse_encoding(handler, data)
-        handler.parse_encoding = parse_encoding
+            return IndexHandler.parse_encoding(mock_handler, data)
+        mock_handler.parse_encoding = parse_encoding
+
+        def detect_encoding(ssh):
+            return IndexHandler.detect_encoding(mock_handler, ssh)
+        mock_handler.detect_encoding = detect_encoding
 
         ssh = Mock(spec=SSHClient)
         stdin = io.BytesIO()
@@ -362,6 +422,184 @@ class TestIndexHandler(unittest.TestCase):
         stderr = io.BytesIO()
         ssh.exec_command.return_value = (stdin, stdout, stderr)
 
-        encoding = IndexHandler.get_default_encoding(handler, ssh)
+        self.assertIsNone(IndexHandler.detect_encoding(mock_handler, ssh))
+
+        encoding = IndexHandler.get_default_encoding(mock_handler, ssh)
         self.assertEqual("utf-8", encoding)
 
+
+class TestLoginRateLimiter(unittest.TestCase):
+
+    def test_blocks_after_too_many_failures(self):
+        limiter = LoginRateLimiter(max_attempts=3, window=300)
+
+        self.assertFalse(limiter.is_blocked('1.1.1.1'))
+        for _ in range(2):
+            limiter.record_failure('1.1.1.1')
+        self.assertFalse(limiter.is_blocked('1.1.1.1'))
+
+        limiter.record_failure('1.1.1.1')
+        self.assertTrue(limiter.is_blocked('1.1.1.1'))
+
+        # other clients are unaffected
+        self.assertFalse(limiter.is_blocked('2.2.2.2'))
+
+    def test_success_resets_the_counter(self):
+        limiter = LoginRateLimiter(max_attempts=2, window=300)
+
+        limiter.record_failure('1.1.1.1')
+        limiter.record_failure('1.1.1.1')
+        self.assertTrue(limiter.is_blocked('1.1.1.1'))
+
+        limiter.reset('1.1.1.1')
+        self.assertFalse(limiter.is_blocked('1.1.1.1'))
+
+    def test_entries_expire_after_the_window(self):
+        limiter = LoginRateLimiter(max_attempts=1, window=300)
+
+        limiter.record_failure('1.1.1.1')
+        self.assertTrue(limiter.is_blocked('1.1.1.1'))
+
+        # pretend the failure happened longer ago than the window
+        count, last = limiter.attempts['1.1.1.1']
+        limiter.attempts['1.1.1.1'] = (count, last - 301)
+
+        self.assertFalse(limiter.is_blocked('1.1.1.1'))
+        self.assertEqual({}, limiter.attempts)
+
+    def test_clear(self):
+        limiter = LoginRateLimiter(max_attempts=1, window=300)
+
+        limiter.record_failure('1.1.1.1')
+        self.assertTrue(limiter.is_blocked('1.1.1.1'))
+        limiter.clear()
+        self.assertFalse(limiter.is_blocked('1.1.1.1'))
+
+
+class TestEncodingCache(unittest.TestCase):
+
+    def test_get_and_set(self):
+        cache = EncodingCache()
+
+        self.assertIsNone(cache.get(('127.0.0.1', 22)))
+        cache.set(('127.0.0.1', 22), 'GBK')
+        self.assertEqual('GBK', cache.get(('127.0.0.1', 22)))
+        # the port is part of the key
+        self.assertIsNone(cache.get(('127.0.0.1', 2200)))
+
+    def test_a_failed_probe_is_not_cached(self):
+        cache = EncodingCache()
+
+        cache.set(('127.0.0.1', 22), None)
+        cache.set(('127.0.0.1', 22), '')
+        self.assertIsNone(cache.get(('127.0.0.1', 22)))
+
+    def test_cache_is_bounded(self):
+        cache = EncodingCache(maxsize=2)
+
+        cache.set(('a', 22), 'UTF-8')
+        cache.set(('b', 22), 'UTF-8')
+        cache.set(('c', 22), 'UTF-8')
+        self.assertEqual(1, len(cache.cache))
+        self.assertEqual('UTF-8', cache.get(('c', 22)))
+
+
+class TestGetEncoding(unittest.TestCase):
+
+    def setUp(self):
+        handler.encoding_cache.clear()
+        self.addCleanup(handler.encoding_cache.clear)
+
+    def make_handler(self, detected):
+        obj = Mock(spec=IndexHandler)
+        obj.detect_encoding.return_value = detected
+        return obj
+
+    def test_probes_once_then_serves_from_cache(self):
+        obj = self.make_handler('GBK')
+        ssh = Mock()
+        dst_addr = ('127.0.0.1', 22)
+
+        self.assertEqual('GBK', IndexHandler.get_encoding(obj, ssh, dst_addr))
+        self.assertEqual('GBK', IndexHandler.get_encoding(obj, ssh, dst_addr))
+        self.assertEqual(1, obj.detect_encoding.call_count)
+
+    def test_failed_detection_falls_back_without_caching(self):
+        obj = self.make_handler(None)
+        ssh = Mock()
+        dst_addr = ('127.0.0.1', 22)
+
+        self.assertEqual(
+            'utf-8', IndexHandler.get_encoding(obj, ssh, dst_addr)
+        )
+        self.assertEqual(
+            'utf-8', IndexHandler.get_encoding(obj, ssh, dst_addr)
+        )
+        self.assertEqual(2, obj.detect_encoding.call_count)
+
+
+class TestClientKey(unittest.TestCase):
+
+    def make_handler(self, auth_manager, addr=('10.0.0.1', 1234)):
+        obj = Mock(spec=MixinHandler)
+        obj.auth_manager = auth_manager
+        obj.get_client_addr.return_value = addr
+        return obj
+
+    def test_logged_in_users_get_their_own_bucket(self):
+        alice = self.make_handler(Mock())
+        alice.current_user = 'alice'
+        bob = self.make_handler(Mock())
+        bob.current_user = 'bob'
+
+        # same source address, different buckets
+        self.assertEqual('user:alice', MixinHandler.get_client_key(alice))
+        self.assertEqual('user:bob', MixinHandler.get_client_key(bob))
+
+    def test_without_auth_the_address_is_used(self):
+        obj = self.make_handler(None)
+        self.assertEqual('10.0.0.1', MixinHandler.get_client_key(obj))
+
+    def test_a_username_cannot_collide_with_an_address(self):
+        spoofer = self.make_handler(Mock())
+        spoofer.current_user = '10.0.0.1'
+
+        direct = self.make_handler(None)
+
+        self.assertNotEqual(
+            MixinHandler.get_client_key(spoofer),
+            MixinHandler.get_client_key(direct)
+        )
+
+
+class TestClearWorker(unittest.TestCase):
+
+    def make_worker(self, worker_id, client_key):
+        obj = Mock()
+        obj.id = worker_id
+        obj.client_key = client_key
+        return obj
+
+    def test_removes_the_worker_and_prunes_empty_buckets(self):
+        a = self.make_worker('a', 'user:admin')
+        b = self.make_worker('b', 'user:admin')
+        clients = {'user:admin': {'a': a, 'b': b}}
+
+        worker.clear_worker(a, clients)
+        self.assertEqual({'user:admin': {'b': b}}, clients)
+
+        worker.clear_worker(b, clients)
+        self.assertEqual({}, clients)
+
+    def test_unregistered_worker_is_ignored(self):
+        orphan = self.make_worker('a', None)
+        clients = {}
+
+        # must not raise even though the worker was never registered
+        worker.clear_worker(orphan, clients)
+        self.assertEqual({}, clients)
+
+        clients = {'user:admin': {'b': 'other'}}
+        orphan.client_key = 'user:admin'
+        worker.clear_worker(orphan, clients)
+        self.assertEqual({'user:admin': {'b': 'other'}}, clients)

tests/test_storage.py

@@ -3,19 +3,27 @@ import shutil
 import tempfile
 import unittest
 
+from webssh.crypto import is_encrypted
 from webssh.storage import ConnectionStore
 
 
+SECRET = 'test-server-secret'
+
+
 class TestConnectionStore(unittest.TestCase):
 
     def setUp(self):
         self.tmpdir = tempfile.mkdtemp()
-        self.store = ConnectionStore(os.path.join(self.tmpdir,
-                                                  'connections.json'))
+        self.filename = os.path.join(self.tmpdir, 'connections.json')
+        self.store = ConnectionStore(self.filename, SECRET)
 
     def tearDown(self):
         shutil.rmtree(self.tmpdir)
 
+    def read_raw(self):
+        with open(self.filename, encoding='utf-8') as f:
+            return f.read()
+
     def test_upsert_list_and_delete(self):
         profile = self.store.upsert('admin', {
             'hostname': '127.0.0.1',
@@ -23,11 +31,12 @@ class TestConnectionStore(unittest.TestCase):
             'username': 'root',
             'password': 'root-secret',
             'term': 'xterm-256color',
+            'directory': '/var/www',
             'auth_type': 'password'
         })
 
         self.assertEqual(profile['title'], 'root@127.0.0.1:22')
-        self.assertEqual(profile['password'], 'root-secret')
+        self.assertEqual(profile['directory'], '/var/www')
         self.assertEqual([profile], self.store.list('admin'))
         self.assertEqual([], self.store.list('other'))
 
@@ -43,7 +52,144 @@ class TestConnectionStore(unittest.TestCase):
         self.assertEqual(profile['id'], updated['id'])
         self.assertEqual(1, len(self.store.list('admin')))
         self.assertEqual('xterm', self.store.list('admin')[0]['term'])
-        self.assertEqual('new-secret', self.store.list('admin')[0]['password'])
+        self.assertEqual('', self.store.list('admin')[0]['directory'])
         self.assertTrue(self.store.delete('admin', profile['id']))
         self.assertFalse(self.store.delete('admin', profile['id']))
         self.assertEqual([], self.store.list('admin'))
+
+    def test_password_never_leaves_the_store(self):
+        profile = self.store.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root',
+            'password': 'root-secret'
+        })
+
+        self.assertNotIn('password', profile)
+        self.assertNotIn('password_enc', profile)
+        self.assertTrue(profile['has_password'])
+
+        listed = self.store.list('admin')[0]
+        self.assertNotIn('password', listed)
+        self.assertNotIn('password_enc', listed)
+        self.assertTrue(listed['has_password'])
+
+        # ... but it is still usable server side
+        self.assertEqual(
+            'root-secret', self.store.get_password('admin', profile['id'])
+        )
+
+    def test_password_is_encrypted_at_rest(self):
+        self.store.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root',
+            'password': 'root-secret'
+        })
+
+        raw = self.read_raw()
+        self.assertNotIn('root-secret', raw)
+        self.assertIn('password_enc', raw)
+
+    def test_password_is_scoped_to_owner_and_destination(self):
+        profile = self.store.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root',
+            'password': 'root-secret'
+        })
+
+        self.assertEqual(
+            '', self.store.get_password('mallory', profile['id'])
+        )
+        self.assertEqual('', self.store.get_password('admin', 'deadbeef'))
+
+        # an edited hostname yields a different id, so nothing is found
+        other_id = self.store.make_id('admin', 'evil.example.com', 22, 'root')
+        self.assertNotEqual(profile['id'], other_id)
+        self.assertEqual('', self.store.get_password('admin', other_id))
+
+    def test_profile_without_password(self):
+        profile = self.store.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root'
+        })
+        self.assertFalse(profile['has_password'])
+        self.assertEqual('', self.store.get_password('admin', profile['id']))
+
+    def test_a_wrong_secret_does_not_break_the_store(self):
+        profile = self.store.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root',
+            'password': 'root-secret'
+        })
+
+        rotated = ConnectionStore(self.filename, 'a-different-secret')
+        self.assertEqual('', rotated.get_password('admin', profile['id']))
+        self.assertTrue(rotated.list('admin')[0]['has_password'])
+
+    def test_encrypt_plaintext_passwords_migrates_old_files(self):
+        legacy = ConnectionStore(self.filename)
+        profile = legacy.upsert('admin', {
+            'hostname': '127.0.0.1',
+            'username': 'root',
+            'password': 'legacy-secret'
+        })
+        self.assertIn('legacy-secret', self.read_raw())
+        self.assertEqual(
+            'legacy-secret', legacy.get_password('admin', profile['id'])
+        )
+
+        self.assertEqual(1, self.store.encrypt_plaintext_passwords())
+
+        raw = self.read_raw()
+        self.assertNotIn('legacy-secret', raw)
+        self.assertEqual(
+            'legacy-secret', self.store.get_password('admin', profile['id'])
+        )
+        self.assertTrue(self.store.list('admin')[0]['has_password'])
+
+        # already migrated, nothing left to do
+        self.assertEqual(0, self.store.encrypt_plaintext_passwords())
+
+    def test_directory_is_normalized_and_optional(self):
+        profile = self.store.upsert('admin', {
+            'hostname': 'example.com',
+            'username': 'deploy',
+            'directory': '  /srv/app  '
+        })
+        self.assertEqual('/srv/app', profile['directory'])
+
+        profile = self.store.upsert('admin', {
+            'hostname': 'example.org',
+            'username': 'deploy'
+        })
+        self.assertEqual('', profile['directory'])
+
+
+class TestSecretBox(unittest.TestCase):
+
+    def test_round_trip(self):
+        from webssh.crypto import SecretBox
+
+        box = SecretBox(SECRET)
+        token = box.encrypt('hunter2')
+        self.assertTrue(is_encrypted(token))
+        self.assertNotIn('hunter2', token)
+        self.assertEqual('hunter2', box.decrypt(token))
+
+    def test_nonce_is_not_reused(self):
+        from webssh.crypto import SecretBox
+
+        box = SecretBox(SECRET)
+        self.assertNotEqual(box.encrypt('same'), box.encrypt('same'))
+
+    def test_empty_and_tampered_values(self):
+        from webssh.crypto import SecretBox
+
+        box = SecretBox(SECRET)
+        self.assertEqual('', box.encrypt(''))
+        self.assertEqual('', box.decrypt(''))
+        self.assertEqual('', box.decrypt('not-a-token'))
+        self.assertEqual('', box.decrypt('aesgcm$####'))
+
+        token = box.encrypt('hunter2')
+        self.assertEqual('', box.decrypt(token[:-4] + 'AAAA'))
+        self.assertEqual('', SecretBox('other').decrypt(token))

tests/test_utils.py

@@ -2,7 +2,8 @@ import unittest
 
 from webssh.utils import (
     is_valid_ip_address, is_valid_port, is_valid_hostname, to_str, to_bytes,
-    to_int, is_ip_hostname, is_same_primary_domain, parse_origin_from_url
+    to_int, is_ip_hostname, is_same_primary_domain, parse_origin_from_url,
+    is_valid_directory, quote_shell_arg, build_cd_command
 )
 
 
@@ -56,6 +57,42 @@ class TestUitls(unittest.TestCase):
         self.assertFalse(is_valid_hostname('127.0.0.1'))
         self.assertFalse(is_valid_hostname('::1'))
 
+    def test_is_valid_directory(self):
+        self.assertTrue(is_valid_directory('/var/www'))
+        self.assertTrue(is_valid_directory('~/projects'))
+        self.assertTrue(is_valid_directory('/tmp/a b'))
+        self.assertTrue(is_valid_directory('/srv/项目'))
+        self.assertTrue(is_valid_directory("/tmp/o'brien"))
+        self.assertFalse(is_valid_directory(''))
+        self.assertFalse(is_valid_directory(None))
+        self.assertFalse(is_valid_directory('/tmp\nrm -rf /'))
+        self.assertFalse(is_valid_directory('/tmp\rwhoami'))
+        self.assertFalse(is_valid_directory('/tmp\x00'))
+        self.assertFalse(is_valid_directory('/tmp\x1b[31m'))
+        self.assertTrue(is_valid_directory('/' + 'a' * 1023))
+        self.assertFalse(is_valid_directory('/' + 'a' * 1024))
+
+    def test_quote_shell_arg(self):
+        self.assertEqual(quote_shell_arg('/var/www'), "'/var/www'")
+        self.assertEqual(quote_shell_arg('/tmp/a b'), "'/tmp/a b'")
+        self.assertEqual(
+            quote_shell_arg('/tmp; rm -rf /'), "'/tmp; rm -rf /'"
+        )
+        self.assertEqual(
+            quote_shell_arg('/tmp/$(whoami)'), "'/tmp/$(whoami)'"
+        )
+        # a single quote is closed, escaped, then reopened
+        self.assertEqual(quote_shell_arg("o'brien"), "'o'\\''brien'")
+        self.assertEqual(
+            quote_shell_arg("'; rm -rf /; '"), "''\\''; rm -rf /; '\\'''"
+        )
+
+    def test_build_cd_command(self):
+        self.assertEqual(build_cd_command('/var/www'), "cd '/var/www'\r")
+        self.assertEqual(
+            build_cd_command('/tmp; reboot'), "cd '/tmp; reboot'\r"
+        )
+
     def test_is_ip_hostname(self):
         self.assertTrue(is_ip_hostname('[::1]'))
         self.assertTrue(is_ip_hostname('127.0.0.1'))