From baed9470400d178a144cc277e9b12745ad69eccd Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Wed, 21 Jan 2026 13:51:13 -0500 Subject: [PATCH 01/14] Secure CLI history files with restrictive permissions on multi-user systems --- awscli/customizations/history/__init__.py | 9 +++++++-- awscli/customizations/history/db.py | 7 +++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index eaa23435b1a4..bc6f2ee9050f 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -53,8 +53,13 @@ def attach_history_handler(session, parsed_args, **kwargs): history_filename = os.environ.get( HISTORY_FILENAME_ENV_VAR, DEFAULT_HISTORY_FILENAME ) - if not os.path.isdir(os.path.dirname(history_filename)): - os.makedirs(os.path.dirname(history_filename)) + + history_dir = os.path.dirname(history_filename) + if not os.path.isdir(history_dir): + os.makedirs(history_dir, mode=0o700) + else: + if os.stat(history_dir).st_uid == os.getuid(): + os.chmod(history_dir, 0o700) connection = DatabaseConnection(history_filename) writer = DatabaseRecordWriter(connection) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index aa0404c08382..78f8a5c28038 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -16,6 +16,7 @@ import threading import time import uuid +import os from botocore.history import BaseHistoryHandler @@ -37,6 +38,12 @@ class DatabaseConnection: _ENABLE_WAL = 'PRAGMA journal_mode=WAL' def __init__(self, db_filename): + if not os.path.exists(db_filename): + open(db_filename, 'a').close() + os.chmod(db_filename, 0o600) + else: + if os.stat(db_filename).st_uid == os.getuid(): + os.chmod(db_filename, 0o600) self._connection = sqlite3.connect( db_filename, check_same_thread=False, isolation_level=None ) From 5d1bb385228bcbf698b7fc696769a59f9b2043b3 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Wed, 21 Jan 2026 16:23:05 -0500 Subject: [PATCH 02/14] Add test coverage for history file permission changes --- awscli/customizations/history/db.py | 2 +- tests/unit/customizations/history/test_db.py | 74 ++++++++++++++++--- .../customizations/history/test_history.py | 51 +++++++++++++ 3 files changed, 117 insertions(+), 10 deletions(-) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index 78f8a5c28038..94a8d10270be 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -13,10 +13,10 @@ import datetime import json import logging +import os import threading import time import uuid -import os from botocore.history import BaseHistoryHandler diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index e8237dd55a3f..a81a8f8a4584 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -48,23 +48,51 @@ def tearDown(self): class TestDatabaseConnection(unittest.TestCase): + def setUp(self): + self.files = FileCreator() + + def tearDown(self): + self.files.remove_all() + + @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_can_connect_to_argument_file(self, mock_connect): + def test_can_connect_to_argument_file( + self, mock_connect, mock_exists, mock_chmod + ): + mock_exists.return_value = True expected_location = os.path.expanduser( os.path.join('~', 'foo', 'bar', 'baz.db') ) - DatabaseConnection(expected_location) + with mock.patch( + 'awscli.customizations.history.db.os.stat' + ) as mock_stat: + mock_stat.return_value.st_uid = os.getuid() + DatabaseConnection(expected_location) mock_connect.assert_called_with( expected_location, check_same_thread=False, isolation_level=None ) + @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_does_try_to_enable_wal(self, mock_connect): - conn = DatabaseConnection(':memory:') + def test_does_try_to_enable_wal( + self, mock_connect, mock_exists, mock_chmod + ): + mock_exists.return_value = True + with mock.patch( + 'awscli.customizations.history.db.os.stat' + ) as mock_stat: + mock_stat.return_value.st_uid = os.getuid() + conn = DatabaseConnection(':memory:') conn._connection.execute.assert_any_call('PRAGMA journal_mode=WAL') - def test_does_ensure_table_created_first(self): - db = DatabaseConnection(":memory:") + @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.path.exists') + def test_does_ensure_table_created_first(self, mock_exists, mock_chmod): + mock_exists.return_value = False + with mock.patch('builtins.open', mock.mock_open()): + db = DatabaseConnection(":memory:") cursor = db.execute('PRAGMA table_info(records)') schema = [col[:3] for col in cursor.fetchall()] expected_schema = [ @@ -77,14 +105,36 @@ def test_does_ensure_table_created_first(self): ] self.assertEqual(expected_schema, schema) + @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_can_close(self, mock_connect): + def test_can_close(self, mock_connect, mock_exists, mock_chmod): + mock_exists.return_value = True connection = mock.Mock() mock_connect.return_value = connection - conn = DatabaseConnection(':memory:') + with mock.patch( + 'awscli.customizations.history.db.os.stat' + ) as mock_stat: + mock_stat.return_value.st_uid = os.getuid() + conn = DatabaseConnection(':memory:') conn.close() self.assertTrue(connection.close.called) + def test_create_new_file_with_secure_permissions(self): + db_filename = os.path.join(self.files.rootdir, 'new_secure.db') + DatabaseConnection(db_filename) + file_mode = os.stat(db_filename).st_mode & 0o777 + self.assertEqual(file_mode, 0o600) + + def test_tighten_existing_file_permissions(self): + db_filename = os.path.join(self.files.rootdir, 'existing.db') + open(db_filename, 'a').close() + os.chmod(db_filename, 0o644) + + DatabaseConnection(db_filename) + file_mode = os.stat(db_filename).st_mode & 0o777 + self.assertEqual(file_mode, 0o600) + class TestDatabaseHistoryHandler(unittest.TestCase): UUID_PATTERN = re.compile( @@ -237,7 +287,13 @@ class BaseDatabaseRecordWriterTester(BaseDatabaseRecordTester): ) def setUp(self): - self.db = DatabaseConnection(':memory:') + with mock.patch( + 'awscli.customizations.history.db.os.path.exists', + return_value=False, + ): + with mock.patch('builtins.open', mock.mock_open()): + with mock.patch('awscli.customizations.history.db.os.chmod'): + self.db = DatabaseConnection(':memory:') self.writer = DatabaseRecordWriter(self.db) diff --git a/tests/unit/customizations/history/test_history.py b/tests/unit/customizations/history/test_history.py index f34830e4893a..300ef2d01209 100644 --- a/tests/unit/customizations/history/test_history.py +++ b/tests/unit/customizations/history/test_history.py @@ -170,6 +170,57 @@ def test_profile_does_not_exist( self.assertFalse(mock_recorder.add_handler.called) self.assertFalse(mock_db_sqlite3.connect.called) + @mock.patch('awscli.customizations.history.sqlite3') + @mock.patch('awscli.customizations.history.db.sqlite3') + @mock.patch( + 'awscli.customizations.history.HISTORY_RECORDER', spec=HistoryRecorder + ) + def test_create_directory_with_secure_permissions( + self, mock_recorder, mock_db_sqlite3, mock_sqlite3 + ): + mock_session = mock.Mock(Session) + mock_session.get_scoped_config.return_value = { + 'cli_history': 'enabled' + } + parsed_args = argparse.Namespace() + parsed_args.command = 's3' + + directory_to_create = os.path.join(self.files.rootdir, 'secure-dir') + db_filename = os.path.join(directory_to_create, 'name.db') + with mock.patch('os.environ', {'AWS_CLI_HISTORY_FILE': db_filename}): + attach_history_handler( + session=mock_session, parsed_args=parsed_args + ) + self.assertTrue(os.path.exists(directory_to_create)) + dir_mode = os.stat(directory_to_create).st_mode & 0o777 + self.assertEqual(dir_mode, 0o700) + + @mock.patch('awscli.customizations.history.sqlite3') + @mock.patch('awscli.customizations.history.db.sqlite3') + @mock.patch( + 'awscli.customizations.history.HISTORY_RECORDER', spec=HistoryRecorder + ) + def test_tighten_existing_directory_permissions( + self, mock_recorder, mock_db_sqlite3, mock_sqlite3 + ): + mock_session = mock.Mock(Session) + mock_session.get_scoped_config.return_value = { + 'cli_history': 'enabled' + } + parsed_args = argparse.Namespace() + parsed_args.command = 's3' + + directory_to_tighten = os.path.join(self.files.rootdir, 'loose-dir') + os.makedirs(directory_to_tighten, mode=0o755) + db_filename = os.path.join(directory_to_tighten, 'name.db') + + with mock.patch('os.environ', {'AWS_CLI_HISTORY_FILE': db_filename}): + attach_history_handler( + session=mock_session, parsed_args=parsed_args + ) + dir_mode = os.stat(directory_to_tighten).st_mode & 0o777 + self.assertEqual(dir_mode, 0o700) + class TestAddHistoryCommand(unittest.TestCase): def test_add_history_command(self): From d827d8a349e9cf2b249f60dd0b07b499a81b51e2 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Wed, 21 Jan 2026 17:06:36 -0500 Subject: [PATCH 03/14] Restrict permissions for telemetry cache directory and session database --- awscli/telemetry.py | 30 ++++++++--- tests/functional/test_telemetry.py | 81 ++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/awscli/telemetry.py b/awscli/telemetry.py index 3cf44e83ace0..90b3381f5e9b 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -77,12 +77,15 @@ class CLISessionDatabaseConnection: _ENABLE_WAL = 'PRAGMA journal_mode=WAL' def __init__(self, connection=None): - self._connection = connection or sqlite3.connect( - _CACHE_DIR / _DATABASE_FILENAME, - check_same_thread=False, - isolation_level=None, - ) - self._ensure_cache_dir() + self._connection = connection + if self._connection is None: + self._ensure_cache_dir() + self._ensure_database_file() + self._connection = sqlite3.connect( + _CACHE_DIR / _DATABASE_FILENAME, + check_same_thread=False, + isolation_level=None, + ) self._ensure_database_setup() def execute(self, query, *parameters): @@ -95,7 +98,20 @@ def execute(self, query, *parameters): return sqlite3.Cursor(self._connection) def _ensure_cache_dir(self): - _CACHE_DIR.mkdir(parents=True, exist_ok=True) + if not _CACHE_DIR.exists(): + _CACHE_DIR.mkdir(parents=True, exist_ok=True, mode=0o700) + else: + if _CACHE_DIR.stat().st_uid == os.getuid(): + os.chmod(_CACHE_DIR, 0o700) + + def _ensure_database_file(self): + db_path = _CACHE_DIR / _DATABASE_FILENAME + if not db_path.exists(): + open(db_path, 'a').close() + os.chmod(db_path, 0o600) + else: + if db_path.stat().st_uid == os.getuid(): + os.chmod(db_path, 0o600) def _ensure_database_setup(self): self._create_session_table() diff --git a/tests/functional/test_telemetry.py b/tests/functional/test_telemetry.py index 698b639d0329..08b226a37555 100644 --- a/tests/functional/test_telemetry.py +++ b/tests/functional/test_telemetry.py @@ -10,7 +10,9 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import os import sqlite3 +import stat from unittest.mock import MagicMock, PropertyMock, patch import pytest @@ -27,6 +29,7 @@ CLISessionOrchestrator, add_session_id_component_to_user_agent_extra, ) +from awscli.testutils import FileCreator from tests.markers import skip_if_windows @@ -130,6 +133,84 @@ def execute(self, query, *parameters): assert cursor.fetchall() == [] +@skip_if_windows +class TestCLISessionDatabaseConnectionPermissions: + def setup_method(self): + self.files = FileCreator() + + def teardown_method(self): + self.files.remove_all() + + def test_create_directory_with_secure_permissions(self): + cache_dir = self.files.full_path('cache') + + with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + from pathlib import Path + + cache_path = Path(cache_dir) + with patch('awscli.telemetry._CACHE_DIR', cache_path): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + assert os.path.isdir(cache_dir) + dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) + assert dir_mode == 0o700 + + def test_tighten_existing_directory_permissions(self): + cache_dir = self.files.full_path('cache') + os.makedirs(cache_dir, mode=0o755) + + with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + from pathlib import Path + + cache_path = Path(cache_dir) + with patch('awscli.telemetry._CACHE_DIR', cache_path): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) + assert dir_mode == 0o700 + + def test_create_database_file_with_secure_permissions(self): + cache_dir = self.files.full_path('cache') + os.makedirs(cache_dir, mode=0o700) + db_file = os.path.join(cache_dir, 'session.db') + + with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + from pathlib import Path + + cache_path = Path(cache_dir) + with patch('awscli.telemetry._CACHE_DIR', cache_path): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + assert os.path.isfile(db_file) + file_mode = stat.S_IMODE(os.stat(db_file).st_mode) + assert file_mode == 0o600 + + def test_tighten_existing_database_file_permissions(self): + cache_dir = self.files.full_path('cache') + os.makedirs(cache_dir, mode=0o700) + db_file = os.path.join(cache_dir, 'session.db') + open(db_file, 'a').close() + os.chmod(db_file, 0o644) + + with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + from pathlib import Path + + cache_path = Path(cache_dir) + with patch('awscli.telemetry._CACHE_DIR', cache_path): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + file_mode = stat.S_IMODE(os.stat(db_file).st_mode) + assert file_mode == 0o600 + + class TestCLISessionDatabaseWriter: def test_write(self, session_writer, session_reader, session_sweeper): session_writer.write( From 4c5b866c515617c7c61a70c12733dfd094931a26 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Wed, 21 Jan 2026 17:33:31 -0500 Subject: [PATCH 04/14] Restrict permissions for autoprompt history directory and file --- awscli/autoprompt/history.py | 12 ++++-- tests/unit/autoprompt/test_history.py | 54 ++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index 55a7c0b68a6b..cce19557992b 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -44,15 +44,21 @@ def load_history_strings(self): def store_string(self, string): history = {'version': self.HISTORY_VERSION, 'commands': []} try: + dir_path = os.path.dirname(self.filename) if os.path.exists(self.filename): with open(self.filename) as f: history = json.load(f) - elif not os.path.exists(os.path.dirname(self.filename)): - os.makedirs(os.path.dirname(self.filename)) + if not os.path.exists(dir_path): + os.makedirs(dir_path, mode=0o700) + else: + if os.stat(dir_path).st_uid == os.getuid(): + os.chmod(dir_path, 0o700) history['commands'].append(string) - history['commands'] = history['commands'][-self._max_commands :] + history['commands'] = history['commands'][-self._max_commands:] with open(self.filename, 'w') as f: json.dump(history, f) + if os.stat(self.filename).st_uid == os.getuid(): + os.chmod(self.filename, 0o600) except Exception: LOG.debug('Exception on loading prompt history:', exc_info=True) diff --git a/tests/unit/autoprompt/test_history.py b/tests/unit/autoprompt/test_history.py index 9c1475a2cc2d..7628e1d69ee3 100644 --- a/tests/unit/autoprompt/test_history.py +++ b/tests/unit/autoprompt/test_history.py @@ -13,6 +13,7 @@ import json import os import shutil +import stat import tempfile from prompt_toolkit.buffer import Buffer @@ -22,7 +23,7 @@ from prompt_toolkit.history import History from awscli.autoprompt.history import HistoryCompleter, HistoryDriver -from awscli.testutils import mock, unittest +from awscli.testutils import mock, skip_if_windows, unittest class TestHistoryCompleter(unittest.TestCase): @@ -190,3 +191,54 @@ def test_handle_io_errors(self, file_history_mock): history_driver = HistoryDriver(self.filename) file_history_mock.store_string.side_effect = IOError history_driver.store_string('aws dynamodb create-table') + + +@skip_if_windows("Permissions tests not applicable on Windows") +class TestHistoryDriverPermissions(unittest.TestCase): + def setUp(self): + self.dirname = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.dirname) + + def test_create_directory_with_secure_permissions(self): + subdir = os.path.join(self.dirname, 'newdir') + filename = os.path.join(subdir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + self.assertTrue(os.path.isdir(subdir)) + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + self.assertEqual(dir_mode, 0o700) + + def test_tighten_existing_directory_permissions(self): + subdir = os.path.join(self.dirname, 'existingdir') + os.makedirs(subdir, mode=0o755) + filename = os.path.join(subdir, 'prompt_history.json') + + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + self.assertEqual(dir_mode, 0o700) + + def test_create_file_with_secure_permissions(self): + filename = os.path.join(self.dirname, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + self.assertTrue(os.path.isfile(filename)) + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + self.assertEqual(file_mode, 0o600) + + def test_tighten_existing_file_permissions(self): + filename = os.path.join(self.dirname, 'prompt_history.json') + with open(filename, 'w') as f: + json.dump({'version': 1, 'commands': []}, f) + os.chmod(filename, 0o644) + + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + self.assertEqual(file_mode, 0o600) From d7ab3bcf9e8fbaa14939694efeadf9dfbd47c5f5 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 09:31:44 -0500 Subject: [PATCH 05/14] Use pytest marker for skip_if_windows in test_history.py --- tests/unit/autoprompt/test_history.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/autoprompt/test_history.py b/tests/unit/autoprompt/test_history.py index 7628e1d69ee3..44d19b029e06 100644 --- a/tests/unit/autoprompt/test_history.py +++ b/tests/unit/autoprompt/test_history.py @@ -23,7 +23,8 @@ from prompt_toolkit.history import History from awscli.autoprompt.history import HistoryCompleter, HistoryDriver -from awscli.testutils import mock, skip_if_windows, unittest +from awscli.testutils import mock, unittest +from tests.markers import skip_if_windows class TestHistoryCompleter(unittest.TestCase): @@ -193,7 +194,7 @@ def test_handle_io_errors(self, file_history_mock): history_driver.store_string('aws dynamodb create-table') -@skip_if_windows("Permissions tests not applicable on Windows") +@skip_if_windows class TestHistoryDriverPermissions(unittest.TestCase): def setUp(self): self.dirname = tempfile.mkdtemp() From a0f35ffba1875a21def2a13ebcef3e2c4903e73a Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 11:55:15 -0500 Subject: [PATCH 06/14] Skip permission tightening on Windows (relies on ACL inheritance) --- awscli/autoprompt/history.py | 11 +++++++---- awscli/customizations/history/__init__.py | 6 ++++-- awscli/customizations/history/db.py | 5 +++-- awscli/telemetry.py | 8 ++++---- tests/unit/customizations/history/test_db.py | 12 ++++++++++-- 5 files changed, 28 insertions(+), 14 deletions(-) diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index cce19557992b..f76a83478d64 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -13,6 +13,7 @@ import json import logging import os +import platform from prompt_toolkit.completion import Completer, Completion from prompt_toolkit.history import FileHistory @@ -49,16 +50,18 @@ def store_string(self, string): with open(self.filename) as f: history = json.load(f) if not os.path.exists(dir_path): - os.makedirs(dir_path, mode=0o700) - else: + os.makedirs(dir_path) + # Restrict access on Unix (Windows relies on ACL inheritance) + if platform.system() != 'Windows': if os.stat(dir_path).st_uid == os.getuid(): os.chmod(dir_path, 0o700) history['commands'].append(string) history['commands'] = history['commands'][-self._max_commands:] with open(self.filename, 'w') as f: json.dump(history, f) - if os.stat(self.filename).st_uid == os.getuid(): - os.chmod(self.filename, 0o600) + if platform.system() != 'Windows': + if os.stat(self.filename).st_uid == os.getuid(): + os.chmod(self.filename, 0o600) except Exception: LOG.debug('Exception on loading prompt history:', exc_info=True) diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index bc6f2ee9050f..6e0be41c5869 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -12,6 +12,7 @@ # language governing permissions and limitations under the License. import logging import os +import platform import sys from botocore.exceptions import ProfileNotFound @@ -56,8 +57,9 @@ def attach_history_handler(session, parsed_args, **kwargs): history_dir = os.path.dirname(history_filename) if not os.path.isdir(history_dir): - os.makedirs(history_dir, mode=0o700) - else: + os.makedirs(history_dir) + # Restrict access on Unix (Windows relies on ACL inheritance) + if platform.system() != 'Windows': if os.stat(history_dir).st_uid == os.getuid(): os.chmod(history_dir, 0o700) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index 94a8d10270be..8c4278d9f4c0 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -14,6 +14,7 @@ import json import logging import os +import platform import threading import time import uuid @@ -40,8 +41,8 @@ class DatabaseConnection: def __init__(self, db_filename): if not os.path.exists(db_filename): open(db_filename, 'a').close() - os.chmod(db_filename, 0o600) - else: + # Restrict access on Unix (Windows relies on ACL inheritance) + if platform.system() != 'Windows': if os.stat(db_filename).st_uid == os.getuid(): os.chmod(db_filename, 0o600) self._connection = sqlite3.connect( diff --git a/awscli/telemetry.py b/awscli/telemetry.py index 90b3381f5e9b..22c24614c250 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -99,8 +99,9 @@ def execute(self, query, *parameters): def _ensure_cache_dir(self): if not _CACHE_DIR.exists(): - _CACHE_DIR.mkdir(parents=True, exist_ok=True, mode=0o700) - else: + _CACHE_DIR.mkdir(parents=True, exist_ok=True) + # Restrict access on Unix (Windows relies on ACL inheritance) + if not is_windows: if _CACHE_DIR.stat().st_uid == os.getuid(): os.chmod(_CACHE_DIR, 0o700) @@ -108,8 +109,7 @@ def _ensure_database_file(self): db_path = _CACHE_DIR / _DATABASE_FILENAME if not db_path.exists(): open(db_path, 'a').close() - os.chmod(db_path, 0o600) - else: + if not is_windows: if db_path.stat().st_uid == os.getuid(): os.chmod(db_path, 0o600) diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index a81a8f8a4584..5d0e09d1df0b 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -88,9 +88,13 @@ def test_does_try_to_enable_wal( conn._connection.execute.assert_any_call('PRAGMA journal_mode=WAL') @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.stat') @mock.patch('awscli.customizations.history.db.os.path.exists') - def test_does_ensure_table_created_first(self, mock_exists, mock_chmod): + def test_does_ensure_table_created_first( + self, mock_exists, mock_stat, mock_chmod + ): mock_exists.return_value = False + mock_stat.return_value.st_uid = os.getuid() with mock.patch('builtins.open', mock.mock_open()): db = DatabaseConnection(":memory:") cursor = db.execute('PRAGMA table_info(records)') @@ -293,7 +297,11 @@ def setUp(self): ): with mock.patch('builtins.open', mock.mock_open()): with mock.patch('awscli.customizations.history.db.os.chmod'): - self.db = DatabaseConnection(':memory:') + with mock.patch( + 'awscli.customizations.history.db.os.stat' + ) as mock_stat: + mock_stat.return_value.st_uid = os.getuid() + self.db = DatabaseConnection(':memory:') self.writer = DatabaseRecordWriter(self.db) From 050e6f6c839c11d819eba543014ed4a271a9c2a6 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 14:36:05 -0500 Subject: [PATCH 07/14] Fix Windows test failures for permission changes --- awscli/customizations/history/db.py | 14 ++-- tests/unit/customizations/history/test_db.py | 65 +++++-------------- .../customizations/history/test_history.py | 8 ++- 3 files changed, 32 insertions(+), 55 deletions(-) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index 8c4278d9f4c0..d12f9105ffee 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -39,12 +39,14 @@ class DatabaseConnection: _ENABLE_WAL = 'PRAGMA journal_mode=WAL' def __init__(self, db_filename): - if not os.path.exists(db_filename): - open(db_filename, 'a').close() - # Restrict access on Unix (Windows relies on ACL inheritance) - if platform.system() != 'Windows': - if os.stat(db_filename).st_uid == os.getuid(): - os.chmod(db_filename, 0o600) + # Skip file operations for in-memory databases + if db_filename != ':memory:' and not db_filename.startswith('file:'): + if not os.path.exists(db_filename): + open(db_filename, 'a').close() + # Restrict access on Unix (Windows relies on ACL inheritance) + if platform.system() != 'Windows': + if os.stat(db_filename).st_uid == os.getuid(): + os.chmod(db_filename, 0o600) self._connection = sqlite3.connect( db_filename, check_same_thread=False, isolation_level=None ) diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index 5d0e09d1df0b..5a3ab27ac7eb 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -15,6 +15,7 @@ import numbers import os import re +import stat import threading from awscli.compat import queue @@ -28,6 +29,7 @@ ) from awscli.testutils import FileCreator, mock, unittest from tests import CaseInsensitiveDict +from tests.markers import skip_if_windows class FakeDatabaseConnection: @@ -55,48 +57,32 @@ def tearDown(self): self.files.remove_all() @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.stat') @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') def test_can_connect_to_argument_file( - self, mock_connect, mock_exists, mock_chmod + self, mock_connect, mock_exists, mock_stat, mock_chmod ): mock_exists.return_value = True + mock_stat.return_value.st_uid = 1000 expected_location = os.path.expanduser( os.path.join('~', 'foo', 'bar', 'baz.db') ) with mock.patch( - 'awscli.customizations.history.db.os.stat' - ) as mock_stat: - mock_stat.return_value.st_uid = os.getuid() + 'awscli.customizations.history.db.os.getuid', return_value=1000 + ): DatabaseConnection(expected_location) mock_connect.assert_called_with( expected_location, check_same_thread=False, isolation_level=None ) - @mock.patch('awscli.customizations.history.db.os.chmod') - @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_does_try_to_enable_wal( - self, mock_connect, mock_exists, mock_chmod - ): - mock_exists.return_value = True - with mock.patch( - 'awscli.customizations.history.db.os.stat' - ) as mock_stat: - mock_stat.return_value.st_uid = os.getuid() - conn = DatabaseConnection(':memory:') + def test_does_try_to_enable_wal(self, mock_connect): + conn = DatabaseConnection(':memory:') conn._connection.execute.assert_any_call('PRAGMA journal_mode=WAL') - @mock.patch('awscli.customizations.history.db.os.chmod') - @mock.patch('awscli.customizations.history.db.os.stat') - @mock.patch('awscli.customizations.history.db.os.path.exists') - def test_does_ensure_table_created_first( - self, mock_exists, mock_stat, mock_chmod - ): - mock_exists.return_value = False - mock_stat.return_value.st_uid = os.getuid() - with mock.patch('builtins.open', mock.mock_open()): - db = DatabaseConnection(":memory:") + def test_does_ensure_table_created_first(self): + db = DatabaseConnection(':memory:') cursor = db.execute('PRAGMA table_info(records)') schema = [col[:3] for col in cursor.fetchall()] expected_schema = [ @@ -109,34 +95,29 @@ def test_does_ensure_table_created_first( ] self.assertEqual(expected_schema, schema) - @mock.patch('awscli.customizations.history.db.os.chmod') - @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_can_close(self, mock_connect, mock_exists, mock_chmod): - mock_exists.return_value = True + def test_can_close(self, mock_connect): connection = mock.Mock() mock_connect.return_value = connection - with mock.patch( - 'awscli.customizations.history.db.os.stat' - ) as mock_stat: - mock_stat.return_value.st_uid = os.getuid() - conn = DatabaseConnection(':memory:') + conn = DatabaseConnection(':memory:') conn.close() self.assertTrue(connection.close.called) + @skip_if_windows def test_create_new_file_with_secure_permissions(self): db_filename = os.path.join(self.files.rootdir, 'new_secure.db') DatabaseConnection(db_filename) - file_mode = os.stat(db_filename).st_mode & 0o777 + file_mode = stat.S_IMODE(os.stat(db_filename).st_mode) self.assertEqual(file_mode, 0o600) + @skip_if_windows def test_tighten_existing_file_permissions(self): db_filename = os.path.join(self.files.rootdir, 'existing.db') open(db_filename, 'a').close() os.chmod(db_filename, 0o644) DatabaseConnection(db_filename) - file_mode = os.stat(db_filename).st_mode & 0o777 + file_mode = stat.S_IMODE(os.stat(db_filename).st_mode) self.assertEqual(file_mode, 0o600) @@ -291,17 +272,7 @@ class BaseDatabaseRecordWriterTester(BaseDatabaseRecordTester): ) def setUp(self): - with mock.patch( - 'awscli.customizations.history.db.os.path.exists', - return_value=False, - ): - with mock.patch('builtins.open', mock.mock_open()): - with mock.patch('awscli.customizations.history.db.os.chmod'): - with mock.patch( - 'awscli.customizations.history.db.os.stat' - ) as mock_stat: - mock_stat.return_value.st_uid = os.getuid() - self.db = DatabaseConnection(':memory:') + self.db = DatabaseConnection(':memory:') self.writer = DatabaseRecordWriter(self.db) diff --git a/tests/unit/customizations/history/test_history.py b/tests/unit/customizations/history/test_history.py index 300ef2d01209..1a8856bc4917 100644 --- a/tests/unit/customizations/history/test_history.py +++ b/tests/unit/customizations/history/test_history.py @@ -12,6 +12,7 @@ # language governing permissions and limitations under the License. import argparse import os +import stat from botocore.exceptions import ProfileNotFound from botocore.history import HistoryRecorder @@ -26,6 +27,7 @@ ) from awscli.customizations.history.db import DatabaseHistoryHandler from awscli.testutils import FileCreator, mock, unittest +from tests.markers import skip_if_windows class TestAttachHistoryHandler(unittest.TestCase): @@ -170,6 +172,7 @@ def test_profile_does_not_exist( self.assertFalse(mock_recorder.add_handler.called) self.assertFalse(mock_db_sqlite3.connect.called) + @skip_if_windows @mock.patch('awscli.customizations.history.sqlite3') @mock.patch('awscli.customizations.history.db.sqlite3') @mock.patch( @@ -192,9 +195,10 @@ def test_create_directory_with_secure_permissions( session=mock_session, parsed_args=parsed_args ) self.assertTrue(os.path.exists(directory_to_create)) - dir_mode = os.stat(directory_to_create).st_mode & 0o777 + dir_mode = stat.S_IMODE(os.stat(directory_to_create).st_mode) self.assertEqual(dir_mode, 0o700) + @skip_if_windows @mock.patch('awscli.customizations.history.sqlite3') @mock.patch('awscli.customizations.history.db.sqlite3') @mock.patch( @@ -218,7 +222,7 @@ def test_tighten_existing_directory_permissions( attach_history_handler( session=mock_session, parsed_args=parsed_args ) - dir_mode = os.stat(directory_to_tighten).st_mode & 0o777 + dir_mode = stat.S_IMODE(os.stat(directory_to_tighten).st_mode) self.assertEqual(dir_mode, 0o700) From 184ce33acd1b515a676371c53b0687cfdfc4871e Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 15:04:13 -0500 Subject: [PATCH 08/14] Simplify DatabaseConnection guard to check only for :memory: --- awscli/customizations/history/db.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index d12f9105ffee..b59adc03678c 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -40,7 +40,7 @@ class DatabaseConnection: def __init__(self, db_filename): # Skip file operations for in-memory databases - if db_filename != ':memory:' and not db_filename.startswith('file:'): + if db_filename != ':memory:': if not os.path.exists(db_filename): open(db_filename, 'a').close() # Restrict access on Unix (Windows relies on ACL inheritance) From 89650f09e5231b298809732212d6d890ec4cca2f Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 15:36:15 -0500 Subject: [PATCH 09/14] Skip permission-checking test on Windows to avoid os.getuid() error --- tests/unit/customizations/history/test_db.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index 5a3ab27ac7eb..4f377d939563 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -56,6 +56,7 @@ def setUp(self): def tearDown(self): self.files.remove_all() + @skip_if_windows @mock.patch('awscli.customizations.history.db.os.chmod') @mock.patch('awscli.customizations.history.db.os.stat') @mock.patch('awscli.customizations.history.db.os.path.exists') @@ -64,14 +65,11 @@ def test_can_connect_to_argument_file( self, mock_connect, mock_exists, mock_stat, mock_chmod ): mock_exists.return_value = True - mock_stat.return_value.st_uid = 1000 + mock_stat.return_value.st_uid = os.getuid() expected_location = os.path.expanduser( os.path.join('~', 'foo', 'bar', 'baz.db') ) - with mock.patch( - 'awscli.customizations.history.db.os.getuid', return_value=1000 - ): - DatabaseConnection(expected_location) + DatabaseConnection(expected_location) mock_connect.assert_called_with( expected_location, check_same_thread=False, isolation_level=None ) From b7241961fcc6be108b47dbd1338d95a1f9737b35 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Thu, 22 Jan 2026 16:55:12 -0500 Subject: [PATCH 10/14] Add Changelog entry --- .changes/next-release/enhancement-clihistory-87056.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/enhancement-clihistory-87056.json diff --git a/.changes/next-release/enhancement-clihistory-87056.json b/.changes/next-release/enhancement-clihistory-87056.json new file mode 100644 index 000000000000..8bc65bcbfa57 --- /dev/null +++ b/.changes/next-release/enhancement-clihistory-87056.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "cli-history", + "description": "Create local history files with specific permissions" +} From 5ce67cff5245f712a4d87f2a03a2347bebe706ac Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Fri, 23 Jan 2026 15:27:55 -0500 Subject: [PATCH 11/14] Address review feedback: simplify Windows handling, fix test patterns, add cached credentials protection --- awscli/autoprompt/history.py | 12 ++-- awscli/botocore/utils.py | 5 ++ awscli/customizations/history/__init__.py | 6 +- awscli/customizations/history/db.py | 7 +- awscli/telemetry.py | 15 +++-- tests/functional/test_telemetry.py | 46 +++++-------- tests/unit/autoprompt/test_history.py | 81 ++++++++++++----------- 7 files changed, 90 insertions(+), 82 deletions(-) diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index f76a83478d64..bcee18f5c2cc 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -13,7 +13,6 @@ import json import logging import os -import platform from prompt_toolkit.completion import Completer, Completion from prompt_toolkit.history import FileHistory @@ -51,17 +50,20 @@ def store_string(self, string): history = json.load(f) if not os.path.exists(dir_path): os.makedirs(dir_path) - # Restrict access on Unix (Windows relies on ACL inheritance) - if platform.system() != 'Windows': + try: if os.stat(dir_path).st_uid == os.getuid(): os.chmod(dir_path, 0o700) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set directory permissions: %s', e) history['commands'].append(string) - history['commands'] = history['commands'][-self._max_commands:] + history['commands'] = history['commands'][-self._max_commands :] with open(self.filename, 'w') as f: json.dump(history, f) - if platform.system() != 'Windows': + try: if os.stat(self.filename).st_uid == os.getuid(): os.chmod(self.filename, 0o600) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set file permissions: %s', e) except Exception: LOG.debug('Exception on loading prompt history:', exc_info=True) diff --git a/awscli/botocore/utils.py b/awscli/botocore/utils.py index d7de8c102c85..c5ae6fb83f43 100644 --- a/awscli/botocore/utils.py +++ b/awscli/botocore/utils.py @@ -3943,6 +3943,11 @@ def __setitem__(self, cache_key, value): ) if not os.path.isdir(self._working_dir): os.makedirs(self._working_dir) + try: + if os.stat(self._working_dir).st_uid == os.getuid(): + os.chmod(self._working_dir, 0o700) + except (OSError, AttributeError): + pass with os.fdopen( os.open(full_key, os.O_WRONLY | os.O_CREAT, 0o600), 'w' ) as f: diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index 6e0be41c5869..102e086cf42e 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -12,7 +12,6 @@ # language governing permissions and limitations under the License. import logging import os -import platform import sys from botocore.exceptions import ProfileNotFound @@ -58,10 +57,11 @@ def attach_history_handler(session, parsed_args, **kwargs): history_dir = os.path.dirname(history_filename) if not os.path.isdir(history_dir): os.makedirs(history_dir) - # Restrict access on Unix (Windows relies on ACL inheritance) - if platform.system() != 'Windows': + try: if os.stat(history_dir).st_uid == os.getuid(): os.chmod(history_dir, 0o700) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set directory permissions: %s', e) connection = DatabaseConnection(history_filename) writer = DatabaseRecordWriter(connection) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index b59adc03678c..aca1e7c00639 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -14,7 +14,6 @@ import json import logging import os -import platform import threading import time import uuid @@ -42,11 +41,13 @@ def __init__(self, db_filename): # Skip file operations for in-memory databases if db_filename != ':memory:': if not os.path.exists(db_filename): + # Create file so we can set permissions before sqlite opens it open(db_filename, 'a').close() - # Restrict access on Unix (Windows relies on ACL inheritance) - if platform.system() != 'Windows': + try: if os.stat(db_filename).st_uid == os.getuid(): os.chmod(db_filename, 0o600) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set file permissions: %s', e) self._connection = sqlite3.connect( db_filename, check_same_thread=False, isolation_level=None ) diff --git a/awscli/telemetry.py b/awscli/telemetry.py index 22c24614c250..b06c69dbd73e 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -11,6 +11,7 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import io +import logging import os import sqlite3 import sys @@ -28,6 +29,8 @@ from awscli.compat import is_windows from awscli.utils import add_component_to_user_agent_extra +LOG = logging.getLogger(__name__) + _CACHE_DIR = Path.home() / '.aws' / 'cli' / 'cache' _DATABASE_FILENAME = 'session.db' _SESSION_LENGTH_SECONDS = 60 * 30 @@ -98,20 +101,22 @@ def execute(self, query, *parameters): return sqlite3.Cursor(self._connection) def _ensure_cache_dir(self): - if not _CACHE_DIR.exists(): - _CACHE_DIR.mkdir(parents=True, exist_ok=True) - # Restrict access on Unix (Windows relies on ACL inheritance) - if not is_windows: + _CACHE_DIR.mkdir(parents=True, exist_ok=True) + try: if _CACHE_DIR.stat().st_uid == os.getuid(): os.chmod(_CACHE_DIR, 0o700) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set directory permissions: %s', e) def _ensure_database_file(self): db_path = _CACHE_DIR / _DATABASE_FILENAME if not db_path.exists(): open(db_path, 'a').close() - if not is_windows: + try: if db_path.stat().st_uid == os.getuid(): os.chmod(db_path, 0o600) + except (OSError, AttributeError) as e: + LOG.debug('Unable to set file permissions: %s', e) def _ensure_database_setup(self): self._create_session_table() diff --git a/tests/functional/test_telemetry.py b/tests/functional/test_telemetry.py index 08b226a37555..0575d2a36e9b 100644 --- a/tests/functional/test_telemetry.py +++ b/tests/functional/test_telemetry.py @@ -13,6 +13,7 @@ import os import sqlite3 import stat +from pathlib import Path from unittest.mock import MagicMock, PropertyMock, patch import pytest @@ -143,15 +144,12 @@ def teardown_method(self): def test_create_directory_with_secure_permissions(self): cache_dir = self.files.full_path('cache') + cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._CACHE_DIR', cache_path): with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): - from pathlib import Path - - cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_path): - conn = CLISessionDatabaseConnection() - conn._connection.close() + conn = CLISessionDatabaseConnection() + conn._connection.close() assert os.path.isdir(cache_dir) dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) @@ -160,32 +158,25 @@ def test_create_directory_with_secure_permissions(self): def test_tighten_existing_directory_permissions(self): cache_dir = self.files.full_path('cache') os.makedirs(cache_dir, mode=0o755) + cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._CACHE_DIR', cache_path): with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): - from pathlib import Path - - cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_path): - conn = CLISessionDatabaseConnection() - conn._connection.close() + conn = CLISessionDatabaseConnection() + conn._connection.close() dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) assert dir_mode == 0o700 def test_create_database_file_with_secure_permissions(self): cache_dir = self.files.full_path('cache') - os.makedirs(cache_dir, mode=0o700) db_file = os.path.join(cache_dir, 'session.db') + cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._CACHE_DIR', cache_path): with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): - from pathlib import Path - - cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_path): - conn = CLISessionDatabaseConnection() - conn._connection.close() + conn = CLISessionDatabaseConnection() + conn._connection.close() assert os.path.isfile(db_file) file_mode = stat.S_IMODE(os.stat(db_file).st_mode) @@ -197,15 +188,12 @@ def test_tighten_existing_database_file_permissions(self): db_file = os.path.join(cache_dir, 'session.db') open(db_file, 'a').close() os.chmod(db_file, 0o644) + cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_dir): + with patch('awscli.telemetry._CACHE_DIR', cache_path): with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): - from pathlib import Path - - cache_path = Path(cache_dir) - with patch('awscli.telemetry._CACHE_DIR', cache_path): - conn = CLISessionDatabaseConnection() - conn._connection.close() + conn = CLISessionDatabaseConnection() + conn._connection.close() file_mode = stat.S_IMODE(os.stat(db_file).st_mode) assert file_mode == 0o600 diff --git a/tests/unit/autoprompt/test_history.py b/tests/unit/autoprompt/test_history.py index 44d19b029e06..0a3637d7cf96 100644 --- a/tests/unit/autoprompt/test_history.py +++ b/tests/unit/autoprompt/test_history.py @@ -16,6 +16,7 @@ import stat import tempfile +import pytest from prompt_toolkit.buffer import Buffer from prompt_toolkit.completion import Completion from prompt_toolkit.document import Document @@ -194,52 +195,58 @@ def test_handle_io_errors(self, file_history_mock): history_driver.store_string('aws dynamodb create-table') +@pytest.fixture +def temp_dir(): + dirname = tempfile.mkdtemp() + yield dirname + shutil.rmtree(dirname) + + @skip_if_windows -class TestHistoryDriverPermissions(unittest.TestCase): - def setUp(self): - self.dirname = tempfile.mkdtemp() +def test_create_directory_with_secure_permissions(temp_dir): + subdir = os.path.join(temp_dir, 'newdir') + filename = os.path.join(subdir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') - def tearDown(self): - shutil.rmtree(self.dirname) + assert os.path.isdir(subdir) + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + assert dir_mode == 0o700 - def test_create_directory_with_secure_permissions(self): - subdir = os.path.join(self.dirname, 'newdir') - filename = os.path.join(subdir, 'prompt_history.json') - history_driver = HistoryDriver(filename) - history_driver.store_string('aws ec2 describe-instances') - self.assertTrue(os.path.isdir(subdir)) - dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) - self.assertEqual(dir_mode, 0o700) +@skip_if_windows +def test_tighten_existing_directory_permissions(temp_dir): + subdir = os.path.join(temp_dir, 'existingdir') + os.makedirs(subdir, mode=0o755) + filename = os.path.join(subdir, 'prompt_history.json') - def test_tighten_existing_directory_permissions(self): - subdir = os.path.join(self.dirname, 'existingdir') - os.makedirs(subdir, mode=0o755) - filename = os.path.join(subdir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') - history_driver = HistoryDriver(filename) - history_driver.store_string('aws ec2 describe-instances') + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + assert dir_mode == 0o700 - dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) - self.assertEqual(dir_mode, 0o700) - def test_create_file_with_secure_permissions(self): - filename = os.path.join(self.dirname, 'prompt_history.json') - history_driver = HistoryDriver(filename) - history_driver.store_string('aws ec2 describe-instances') +@skip_if_windows +def test_create_file_with_secure_permissions(temp_dir): + filename = os.path.join(temp_dir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') - self.assertTrue(os.path.isfile(filename)) - file_mode = stat.S_IMODE(os.stat(filename).st_mode) - self.assertEqual(file_mode, 0o600) + assert os.path.isfile(filename) + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + assert file_mode == 0o600 - def test_tighten_existing_file_permissions(self): - filename = os.path.join(self.dirname, 'prompt_history.json') - with open(filename, 'w') as f: - json.dump({'version': 1, 'commands': []}, f) - os.chmod(filename, 0o644) - history_driver = HistoryDriver(filename) - history_driver.store_string('aws ec2 describe-instances') +@skip_if_windows +def test_tighten_existing_file_permissions(temp_dir): + filename = os.path.join(temp_dir, 'prompt_history.json') + with open(filename, 'w') as f: + json.dump({'version': 1, 'commands': []}, f) + os.chmod(filename, 0o644) + + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') - file_mode = stat.S_IMODE(os.stat(filename).st_mode) - self.assertEqual(file_mode, 0o600) + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + assert file_mode == 0o600 From 4548f1e1fbfc2e5a6ce6555160d6fed0a6faf5dc Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Fri, 23 Jan 2026 17:55:41 -0500 Subject: [PATCH 12/14] Address review feedback: remove UID checks, add logging to botocore --- awscli/autoprompt/history.py | 6 ++---- awscli/botocore/utils.py | 7 +++---- awscli/customizations/history/__init__.py | 3 +-- awscli/customizations/history/db.py | 3 +-- awscli/telemetry.py | 6 ++---- 5 files changed, 9 insertions(+), 16 deletions(-) diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index bcee18f5c2cc..639d320902d6 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -51,8 +51,7 @@ def store_string(self, string): if not os.path.exists(dir_path): os.makedirs(dir_path) try: - if os.stat(dir_path).st_uid == os.getuid(): - os.chmod(dir_path, 0o700) + os.chmod(dir_path, 0o700) except (OSError, AttributeError) as e: LOG.debug('Unable to set directory permissions: %s', e) history['commands'].append(string) @@ -60,8 +59,7 @@ def store_string(self, string): with open(self.filename, 'w') as f: json.dump(history, f) try: - if os.stat(self.filename).st_uid == os.getuid(): - os.chmod(self.filename, 0o600) + os.chmod(self.filename, 0o600) except (OSError, AttributeError) as e: LOG.debug('Unable to set file permissions: %s', e) except Exception: diff --git a/awscli/botocore/utils.py b/awscli/botocore/utils.py index c5ae6fb83f43..c821e6f0e01d 100644 --- a/awscli/botocore/utils.py +++ b/awscli/botocore/utils.py @@ -3944,10 +3944,9 @@ def __setitem__(self, cache_key, value): if not os.path.isdir(self._working_dir): os.makedirs(self._working_dir) try: - if os.stat(self._working_dir).st_uid == os.getuid(): - os.chmod(self._working_dir, 0o700) - except (OSError, AttributeError): - pass + os.chmod(self._working_dir, 0o700) + except (OSError, AttributeError) as e: + logger.debug('Unable to set directory permissions: %s', e) with os.fdopen( os.open(full_key, os.O_WRONLY | os.O_CREAT, 0o600), 'w' ) as f: diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index 102e086cf42e..a79694657405 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -58,8 +58,7 @@ def attach_history_handler(session, parsed_args, **kwargs): if not os.path.isdir(history_dir): os.makedirs(history_dir) try: - if os.stat(history_dir).st_uid == os.getuid(): - os.chmod(history_dir, 0o700) + os.chmod(history_dir, 0o700) except (OSError, AttributeError) as e: LOG.debug('Unable to set directory permissions: %s', e) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index aca1e7c00639..b2fa06ff2d03 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -44,8 +44,7 @@ def __init__(self, db_filename): # Create file so we can set permissions before sqlite opens it open(db_filename, 'a').close() try: - if os.stat(db_filename).st_uid == os.getuid(): - os.chmod(db_filename, 0o600) + os.chmod(db_filename, 0o600) except (OSError, AttributeError) as e: LOG.debug('Unable to set file permissions: %s', e) self._connection = sqlite3.connect( diff --git a/awscli/telemetry.py b/awscli/telemetry.py index b06c69dbd73e..18f98a94714f 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -103,8 +103,7 @@ def execute(self, query, *parameters): def _ensure_cache_dir(self): _CACHE_DIR.mkdir(parents=True, exist_ok=True) try: - if _CACHE_DIR.stat().st_uid == os.getuid(): - os.chmod(_CACHE_DIR, 0o700) + os.chmod(_CACHE_DIR, 0o700) except (OSError, AttributeError) as e: LOG.debug('Unable to set directory permissions: %s', e) @@ -113,8 +112,7 @@ def _ensure_database_file(self): if not db_path.exists(): open(db_path, 'a').close() try: - if db_path.stat().st_uid == os.getuid(): - os.chmod(db_path, 0o600) + os.chmod(db_path, 0o600) except (OSError, AttributeError) as e: LOG.debug('Unable to set file permissions: %s', e) From 6f072bad67df9c17e5d7a329b7f07d567b8c24f6 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Mon, 26 Jan 2026 15:32:40 -0500 Subject: [PATCH 13/14] Remove AttributeError from exception handling after removing getuid check --- awscli/autoprompt/history.py | 4 ++-- awscli/botocore/utils.py | 2 +- awscli/customizations/history/__init__.py | 2 +- awscli/customizations/history/db.py | 2 +- awscli/telemetry.py | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index 639d320902d6..af36357704cd 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -52,7 +52,7 @@ def store_string(self, string): os.makedirs(dir_path) try: os.chmod(dir_path, 0o700) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set directory permissions: %s', e) history['commands'].append(string) history['commands'] = history['commands'][-self._max_commands :] @@ -60,7 +60,7 @@ def store_string(self, string): json.dump(history, f) try: os.chmod(self.filename, 0o600) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set file permissions: %s', e) except Exception: LOG.debug('Exception on loading prompt history:', exc_info=True) diff --git a/awscli/botocore/utils.py b/awscli/botocore/utils.py index c821e6f0e01d..4323862f2202 100644 --- a/awscli/botocore/utils.py +++ b/awscli/botocore/utils.py @@ -3945,7 +3945,7 @@ def __setitem__(self, cache_key, value): os.makedirs(self._working_dir) try: os.chmod(self._working_dir, 0o700) - except (OSError, AttributeError) as e: + except OSError as e: logger.debug('Unable to set directory permissions: %s', e) with os.fdopen( os.open(full_key, os.O_WRONLY | os.O_CREAT, 0o600), 'w' diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index a79694657405..37a635791015 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -59,7 +59,7 @@ def attach_history_handler(session, parsed_args, **kwargs): os.makedirs(history_dir) try: os.chmod(history_dir, 0o700) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set directory permissions: %s', e) connection = DatabaseConnection(history_filename) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index b2fa06ff2d03..255b54bfc322 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -45,7 +45,7 @@ def __init__(self, db_filename): open(db_filename, 'a').close() try: os.chmod(db_filename, 0o600) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set file permissions: %s', e) self._connection = sqlite3.connect( db_filename, check_same_thread=False, isolation_level=None diff --git a/awscli/telemetry.py b/awscli/telemetry.py index 18f98a94714f..d46c07024b5c 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -104,7 +104,7 @@ def _ensure_cache_dir(self): _CACHE_DIR.mkdir(parents=True, exist_ok=True) try: os.chmod(_CACHE_DIR, 0o700) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set directory permissions: %s', e) def _ensure_database_file(self): @@ -113,7 +113,7 @@ def _ensure_database_file(self): open(db_path, 'a').close() try: os.chmod(db_path, 0o600) - except (OSError, AttributeError) as e: + except OSError as e: LOG.debug('Unable to set file permissions: %s', e) def _ensure_database_setup(self): From 1f459975dd9d9f42adaa3ee48a98838c7770a1f1 Mon Sep 17 00:00:00 2001 From: Andrew Asseily Date: Mon, 26 Jan 2026 15:36:55 -0500 Subject: [PATCH 14/14] Remove os.stat mock from test after removing UID check from code --- tests/unit/customizations/history/test_db.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index 4f377d939563..7ee8f14970c9 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -58,14 +58,12 @@ def tearDown(self): @skip_if_windows @mock.patch('awscli.customizations.history.db.os.chmod') - @mock.patch('awscli.customizations.history.db.os.stat') @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') def test_can_connect_to_argument_file( - self, mock_connect, mock_exists, mock_stat, mock_chmod + self, mock_connect, mock_exists, mock_chmod ): mock_exists.return_value = True - mock_stat.return_value.st_uid = os.getuid() expected_location = os.path.expanduser( os.path.join('~', 'foo', 'bar', 'baz.db') )