From 9acfd0ce9ca50668a0ffe88355c15098b16865e0 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Wed, 10 Jun 2026 20:13:20 +0200 Subject: [PATCH 1/4] Add Settings.getsecret() --- docs/topics/settings.rst | 129 ++++++++++++ scrapy/core/downloader/handlers/ftp.py | 2 +- scrapy/core/downloader/handlers/s3.py | 6 +- scrapy/crawler.py | 5 + scrapy/extensions/feedexport.py | 6 +- scrapy/pipelines/files.py | 8 +- scrapy/settings/__init__.py | 16 +- scrapy/settings/default_settings.py | 5 + scrapy/utils/secrets.py | 153 ++++++++++++++ tests/test_settings/__init__.py | 76 +++++++ tests/test_utils_secrets.py | 263 +++++++++++++++++++++++++ 11 files changed, 657 insertions(+), 12 deletions(-) create mode 100644 scrapy/utils/secrets.py create mode 100644 tests/test_utils_secrets.py diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 06de33e6f..1c141f489 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -236,6 +236,87 @@ The ``settings`` object can be used like a :class:`dict` (e.g. which may be passed from the command line as strings, it is recommended to use one of the methods provided by the :class:`~scrapy.settings.Settings` API. +.. _secret-settings: + +Secret settings +=============== + +Some settings hold sensitive values such as API keys, passwords, or tokens. +Scrapy reads those settings with +:meth:`~scrapy.settings.BaseSettings.getsecret`, which supports a JSON +reference syntax that lets you store the actual secret outside your settings +file. + +Plain string values are returned as-is: + +.. code-block:: python + + AWS_ACCESS_KEY_ID = "AKIAIOSFODNN7EXAMPLE" + +To read the secret from an environment variable instead, set the value to a +JSON object with an ``"env"`` key: + +.. code-block:: python + + AWS_ACCESS_KEY_ID = '{"env": "AWS_ACCESS_KEY_ID"}' + +If the environment variable is not set, ``getsecret()`` logs a warning and the +default setting value is used instead. + +To read the secret from the system keyring (requires the keyring_ package), +use a ``"keyring"`` key. The value can be a username string — in which case +the service name defaults to ``"scrapy"`` — or an object for full control: + +.. _keyring: https://pypi.org/project/keyring/ + +.. code-block:: python + + # short form — service name defaults to "scrapy" + AWS_ACCESS_KEY_ID = '{"keyring": "my-aws-account"}' + + # long form + AWS_ACCESS_KEY_ID = '{"keyring": {"username": "my-aws-account", "service": "myapp", "backend": "onepassword_keyring.OnePasswordKeyring"}}' + +If the keyring entry does not exist, components that use the secret setting may +treat the issue as a user error and e.g. disable themselves or even stop the +spider. + +If a secret value is itself a JSON object (e.g. a GCP service-account blob), +use the ``"raw"`` key to prevent the object from being misinterpreted as a +source reference: + +.. code-block:: python + + GCP_CREDENTIALS = ( + '{"raw": {"type": "service_account", "project_id": "my-project", ...}}' + ) + +The inner value is returned as-is if it is a string, or JSON-serialised +otherwise. + +You can also combine the ``"env"`` syntax with a JSON secret: store the +JSON-encoded blob in the environment variable and reference it with +``{"env": "MY_VAR"}``. + +.. rubric:: Loading secrets from a :file:`.env` file + +Use :setting:`DOTENV_PATH` to automatically load a :file:`.env` file at the +start of each crawl. This pairs naturally with the ``"env"`` reference syntax: + +.. code-block:: ini + + # .env (keep out of version control) + AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE + AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY + +.. code-block:: python + + # settings.py + AWS_ACCESS_KEY_ID = '{"env": "AWS_ACCESS_KEY_ID"}' + AWS_SECRET_ACCESS_KEY = '{"env": "AWS_SECRET_ACCESS_KEY"}' + +Real environment variables always take precedence over :file:`.env` values +(see :setting:`DOTENV_OVERRIDE`). .. _component-priority-dictionaries: @@ -394,6 +475,8 @@ Default: ``None`` The AWS access key used by code that requires access to `Amazon Web services`_, such as the :ref:`S3 feed storage backend `. +This is a :ref:`secret setting `. + .. setting:: AWS_SECRET_ACCESS_KEY AWS_SECRET_ACCESS_KEY @@ -404,6 +487,8 @@ Default: ``None`` The AWS secret key used by code that requires access to `Amazon Web services`_, such as the :ref:`S3 feed storage backend `. +This is a :ref:`secret setting `. + .. setting:: AWS_SESSION_TOKEN AWS_SESSION_TOKEN @@ -415,6 +500,8 @@ The AWS security token used by code that requires access to `Amazon Web services such as the :ref:`S3 feed storage backend `, when using `temporary security credentials`_. +This is a :ref:`secret setting `. + .. _temporary security credentials: https://docs.aws.amazon.com/IAM/latest/UserGuide/security-creds.html .. setting:: AWS_ENDPOINT_URL @@ -703,6 +790,46 @@ Timeout for processing of DNS queries in seconds. Float is supported. :setting:`TWISTED_REACTOR_ENABLED` is ``False``, and may have no effect either when :setting:`DNS_RESOLVER` is set to a different resolver. +.. setting:: DOTENV_OVERRIDE + +DOTENV_OVERRIDE +--------------- + +Default: ``False`` + +If ``True``, variables loaded from the :setting:`DOTENV_PATH` file override +existing environment variables. The default ``False`` ensures that real +environment variables always take precedence over the file. + +.. setting:: DOTENV_PATH + +DOTENV_PATH +----------- + +Default: ``".env"`` + +Path to a :file:`.env` file that is loaded into the environment at the start of +each crawl (via :class:`~scrapy.crawler.Crawler`). Relative paths are resolved +from the current working directory. If the file does not exist the setting is +silently ignored. + +Loading a :file:`.env` file requires the python-dotenv_ package:: + + pip install python-dotenv + +.. _python-dotenv: https://pypi.org/project/python-dotenv/ + +Use this in combination with :meth:`~scrapy.settings.BaseSettings.getsecret` +to keep secrets out of your settings files:: + + # settings.py + ZYTE_API_KEY = '{"env": "ZYTE_API_KEY"}' + + # .env (not committed to version control) + ZYTE_API_KEY=your-actual-key + +See also :setting:`DOTENV_OVERRIDE`. + .. setting:: DOWNLOADER DOWNLOADER @@ -1352,6 +1479,8 @@ Default: ``"guest"`` The password to use for FTP connections when there is no ``"ftp_password"`` in ``Request`` meta. +This is a :ref:`secret setting `. + .. note:: Paraphrasing `RFC 1635`_, although it is common to use either the password "guest" or one's e-mail address for anonymous FTP, diff --git a/scrapy/core/downloader/handlers/ftp.py b/scrapy/core/downloader/handlers/ftp.py index 6258067c1..a0b6e2a23 100644 --- a/scrapy/core/downloader/handlers/ftp.py +++ b/scrapy/core/downloader/handlers/ftp.py @@ -89,7 +89,7 @@ class FTPDownloadHandler(BaseDownloadHandler): raise NotConfigured(f"{type(self).__name__} requires a Twisted reactor.") super().__init__(crawler) self.default_user = crawler.settings["FTP_USER"] - self.default_password = crawler.settings["FTP_PASSWORD"] + self.default_password = crawler.settings.getsecret("FTP_PASSWORD") self.passive_mode = crawler.settings["FTP_PASSIVE_MODE"] async def download_request(self, request: Request) -> Response: diff --git a/scrapy/core/downloader/handlers/s3.py b/scrapy/core/downloader/handlers/s3.py index c6660ff48..a1ede0cfd 100644 --- a/scrapy/core/downloader/handlers/s3.py +++ b/scrapy/core/downloader/handlers/s3.py @@ -24,9 +24,9 @@ class S3DownloadHandler(BaseDownloadHandler): raise NotConfigured("missing botocore library") super().__init__(crawler) - aws_access_key_id = crawler.settings["AWS_ACCESS_KEY_ID"] - aws_secret_access_key = crawler.settings["AWS_SECRET_ACCESS_KEY"] - aws_session_token = crawler.settings["AWS_SESSION_TOKEN"] + aws_access_key_id = crawler.settings.getsecret("AWS_ACCESS_KEY_ID") + aws_secret_access_key = crawler.settings.getsecret("AWS_SECRET_ACCESS_KEY") + aws_session_token = crawler.settings.getsecret("AWS_SESSION_TOKEN") self.anon = not aws_access_key_id and not aws_secret_access_key self._signer = None if not self.anon: diff --git a/scrapy/crawler.py b/scrapy/crawler.py index 8e3ae7879..bb44cb019 100644 --- a/scrapy/crawler.py +++ b/scrapy/crawler.py @@ -40,6 +40,7 @@ from scrapy.utils.reactor import ( verify_installed_reactor, ) from scrapy.utils.reactorless import install_reactor_import_hook +from scrapy.utils.secrets import _load_dotenv if TYPE_CHECKING: from collections.abc import Awaitable, Generator, Iterable @@ -70,6 +71,10 @@ class Crawler: self.spidercls: type[Spider] = spidercls self.settings: Settings = settings.copy() self.spidercls.update_settings(self.settings) + _load_dotenv( + path=self.settings.get("DOTENV_PATH", ".env"), + override=self.settings.getbool("DOTENV_OVERRIDE", False), + ) self._update_root_log_handler() self.addons: AddonManager = AddonManager(self) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 8029f85c9..82561bcce 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -239,9 +239,9 @@ class S3FeedStorage(BlockingFeedStorage): ) -> Self: return cls( uri, - access_key=crawler.settings["AWS_ACCESS_KEY_ID"], - secret_key=crawler.settings["AWS_SECRET_ACCESS_KEY"], - session_token=crawler.settings["AWS_SESSION_TOKEN"], + access_key=crawler.settings.getsecret("AWS_ACCESS_KEY_ID"), + secret_key=crawler.settings.getsecret("AWS_SECRET_ACCESS_KEY"), + session_token=crawler.settings.getsecret("AWS_SESSION_TOKEN"), acl=crawler.settings["FEED_STORAGE_S3_ACL"] or None, endpoint_url=crawler.settings["AWS_ENDPOINT_URL"] or None, region_name=crawler.settings["AWS_REGION_NAME"] or None, diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index 0066fd38f..f856c3555 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -511,9 +511,9 @@ class FilesPipeline(MediaPipeline): s3store: type[S3FilesStore] = cast( "type[S3FilesStore]", cls.STORE_SCHEMES["s3"] ) - s3store.AWS_ACCESS_KEY_ID = settings["AWS_ACCESS_KEY_ID"] - s3store.AWS_SECRET_ACCESS_KEY = settings["AWS_SECRET_ACCESS_KEY"] - s3store.AWS_SESSION_TOKEN = settings["AWS_SESSION_TOKEN"] + s3store.AWS_ACCESS_KEY_ID = settings.getsecret("AWS_ACCESS_KEY_ID") + s3store.AWS_SECRET_ACCESS_KEY = settings.getsecret("AWS_SECRET_ACCESS_KEY") + s3store.AWS_SESSION_TOKEN = settings.getsecret("AWS_SESSION_TOKEN") s3store.AWS_ENDPOINT_URL = settings["AWS_ENDPOINT_URL"] s3store.AWS_REGION_NAME = settings["AWS_REGION_NAME"] s3store.AWS_USE_SSL = settings["AWS_USE_SSL"] @@ -530,7 +530,7 @@ class FilesPipeline(MediaPipeline): "type[FTPFilesStore]", cls.STORE_SCHEMES["ftp"] ) ftp_store.FTP_USERNAME = settings["FTP_USER"] - ftp_store.FTP_PASSWORD = settings["FTP_PASSWORD"] + ftp_store.FTP_PASSWORD = settings.getsecret("FTP_PASSWORD") ftp_store.USE_ACTIVE_MODE = settings.getbool("FEED_STORAGE_FTP_ACTIVE") def _get_store(self, uri: str) -> FilesStoreProtocol: diff --git a/scrapy/settings/__init__.py b/scrapy/settings/__init__.py index be298e9b1..b1c503a60 100644 --- a/scrapy/settings/__init__.py +++ b/scrapy/settings/__init__.py @@ -13,6 +13,7 @@ from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.settings import default_settings from scrapy.utils.misc import load_object from scrapy.utils.python import global_object_name +from scrapy.utils.secrets import resolve_secret logger = getLogger(__name__) @@ -85,7 +86,7 @@ class BaseSettings(MutableMapping[str, Any]): Key-value entries can be passed on initialization with the ``values`` argument, and they would take the ``priority`` level (unless ``values`` is already an instance of :class:`~scrapy.settings.BaseSettings`, in which - case the existing priority levels will be kept). If the ``priority`` + case the existing priority levels will be kept). If the ``priority`` argument is a string, the priority name will be looked up in :attr:`~scrapy.settings.SETTINGS_PRIORITIES`. Otherwise, a specific integer should be provided. @@ -316,6 +317,19 @@ class BaseSettings(MutableMapping[str, Any]): ) return copy.deepcopy(value) + def getsecret(self, name: str, default: str | None = None) -> str | None: + """Get a setting value as a secret string. + + Looks up *name* and resolves its value via + :func:`~scrapy.utils.secrets.resolve_secret`. If the setting is not + defined or its value is ``None``, *default* is returned without calling + :func:`~scrapy.utils.secrets.resolve_secret`. + """ + value = self.get(name) + if value is None: + return default + return resolve_secret(value, default) + def getwithbase(self, name: str) -> BaseSettings: """Get a composition of a dictionary-like setting and its ``_BASE`` counterpart. diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 7d5612026..c943a2dbf 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -50,6 +50,8 @@ __all__ = [ "DNSCACHE_ENABLED", "DNSCACHE_SIZE", "DNS_TIMEOUT", + "DOTENV_OVERRIDE", + "DOTENV_PATH", "DOWNLOADER", "DOWNLOADER_CLIENTCONTEXTFACTORY", "DOWNLOADER_CLIENT_TLS_CIPHERS", @@ -248,6 +250,9 @@ DNSCACHE_SIZE = 10000 DNS_RESOLVER = "scrapy.resolver.CachingThreadedResolver" DNS_TIMEOUT = 60 +DOTENV_OVERRIDE = False +DOTENV_PATH = ".env" + DOWNLOAD_BIND_ADDRESS = None DOWNLOAD_DELAY = 0 diff --git a/scrapy/utils/secrets.py b/scrapy/utils/secrets.py new file mode 100644 index 000000000..7329cf9aa --- /dev/null +++ b/scrapy/utils/secrets.py @@ -0,0 +1,153 @@ +from __future__ import annotations + +import json +import os +import warnings +from logging import getLogger +from pathlib import Path +from typing import Any + +from scrapy.utils.misc import load_object + +logger = getLogger(__name__) + + +def resolve_secret(value: Any, default: str | None = None) -> str | None: + """Resolve a raw value as a secret string. + + Plain strings and non-JSON values are returned as-is. JSON object values + that contain a recognised source key are resolved against that source: + + - ``{"env": "VAR"}`` — read from the *VAR* environment variable. If the + variable is not set a warning is logged and *default* is returned. + + - ``{"keyring": "username"}`` or ``{"keyring": {"username": "…", + "service": "…", "backend": "…"}}`` — read from the system keyring + (requires the keyring_ package; default service is ``"scrapy"``). If + the entry does not exist a :exc:`KeyError` is raised. + + .. _keyring: https://pypi.org/project/keyring/ + + - ``{"raw": }`` — return the inner value directly (JSON-serialised + if it is not a string). Use this as an escape hatch for literal JSON + secrets whose keys would otherwise be misinterpreted as a source + reference. + + Any other JSON value or object is returned as the original raw string, + preserving backward compatibility. + + See :ref:`secret-settings` for full usage examples. + + :param value: the raw setting value to resolve. + :param default: value to return when an ``"env"`` reference cannot be + resolved because the environment variable is not set. Defaults to + ``None``. + """ + if isinstance(value, dict): + spec: dict[str, Any] = value + raw: str | None = None + elif isinstance(value, str): + raw = value + try: + parsed = json.loads(value) + except (ValueError, TypeError): + return value + if not isinstance(parsed, dict): + # JSON but not an object (string, number, bool, list) — treat as + # a plain string secret. + return value + spec = parsed + else: + return str(value) + + if "raw" in spec: + inner = spec["raw"] + return inner if isinstance(inner, str) else json.dumps(inner) + + if "env" in spec: + return _resolve_env(spec, default) + + if "keyring" in spec: + return _resolve_keyring(spec) + + # JSON object with no recognised source key — it is a literal secret (e.g. + # a JSON blob stored directly in settings). + return raw + + +def _resolve_env(spec: dict[str, Any], default: str | None) -> str | None: + var = spec["env"] + if not isinstance(var, str): + raise ValueError(f'"env" must be a string, got {type(var).__name__!r}') + value = os.environ.get(var) + if value is None: + logger.warning( + f"Environment variable {var!r} referenced in a secret reference " + "value is not set; treating as undefined." + ) + return default + return value + + +def _resolve_keyring(spec: dict[str, Any]) -> str: + try: + import keyring # noqa: PLC0415 + except ImportError: + raise ImportError( + "Install the 'keyring' package to use keyring-based secrets: " + "pip install keyring" + ) from None + + kr = spec["keyring"] + if isinstance(kr, str): + username: str = kr + service: str = "scrapy" + backend_path: str | None = None + elif isinstance(kr, dict): + if "username" not in kr: + raise ValueError('"keyring" object must have a "username" key') + username = kr["username"] + service = kr.get("service", "scrapy") + backend_path = kr.get("backend") + else: + raise ValueError('"keyring" must be a string or an object') + + if backend_path is not None: + backend_cls = load_object(backend_path) + value: str | None = backend_cls().get_password(service, username) + else: + value = keyring.get_password(service, username) + + if value is None: + raise KeyError( + f"No keyring entry found for service={service!r}, username={username!r}" + ) + return value + + +def _load_dotenv(path: str = ".env", override: bool = False) -> None: + """Load environment variables from a :file:`.env` file into :data:`os.environ`. + + The file is resolved relative to the current working directory. If it + does not exist the call is a no-op. Requires the ``python-dotenv`` + package; if the package is not installed a :exc:`UserWarning` is emitted + and the call returns without raising. + + :param path: path to the :file:`.env` file. Defaults to ``".env"``. + :param override: when ``True``, values from the file override existing + environment variables. Defaults to ``False`` so that real environment + variables always take precedence over the file. + """ + if not Path(path).is_file(): + return + try: + from dotenv import load_dotenv as _load # noqa: PLC0415 + except ImportError: + warnings.warn( + "Install the 'python-dotenv' package to load a .env file: " + "pip install python-dotenv", + UserWarning, + stacklevel=2, + ) + return + _load(dotenv_path=path, override=override) diff --git a/tests/test_settings/__init__.py b/tests/test_settings/__init__.py index 7b3c52d65..71893412e 100644 --- a/tests/test_settings/__init__.py +++ b/tests/test_settings/__init__.py @@ -331,6 +331,82 @@ class TestBaseSettings: ): settings.getbool("TEST_DISABLED_WRONG") + def test_getsecret_plain_string(self): + settings = BaseSettings({"SECRET": "my-api-key"}, priority="project") + assert settings.getsecret("SECRET") == "my-api-key" + + def test_getsecret_not_defined_returns_none(self): + settings = BaseSettings() + assert settings.getsecret("MISSING_SECRET") is None + + def test_getsecret_not_defined_returns_default(self): + settings = BaseSettings() + assert settings.getsecret("MISSING_SECRET", default="fallback") == "fallback" + + def test_getsecret_env_var(self): + settings = BaseSettings( + {"SECRET": '{"env": "TEST_SECRET_VAR"}'}, priority="project" + ) + with mock.patch.dict("os.environ", {"TEST_SECRET_VAR": "env-value"}): + assert settings.getsecret("SECRET") == "env-value" + + def test_getsecret_env_var_missing_returns_none(self, caplog): + settings = BaseSettings( + {"SECRET": '{"env": "ABSENT_SECRET_VAR"}'}, priority="project" + ) + with ( + mock.patch.dict("os.environ", {}, clear=True), + caplog.at_level(logging.WARNING), + ): + result = settings.getsecret("SECRET") + assert result is None + assert "ABSENT_SECRET_VAR" in caplog.text + + def test_getsecret_env_var_missing_uses_default(self, caplog): + settings = BaseSettings( + {"SECRET": '{"env": "ABSENT_SECRET_VAR"}'}, priority="project" + ) + with ( + mock.patch.dict("os.environ", {}, clear=True), + caplog.at_level(logging.WARNING), + ): + result = settings.getsecret("SECRET", default="fallback") + assert result == "fallback" + + def test_getsecret_raw(self): + settings = BaseSettings( + {"SECRET": '{"raw": "literal-value"}'}, priority="project" + ) + assert settings.getsecret("SECRET") == "literal-value" + + def test_getsecret_json_object_literal(self): + # A JSON object with no recognised key is a literal secret. + blob = '{"type": "service_account", "project_id": "proj"}' + settings = BaseSettings({"SECRET": blob}, priority="project") + assert settings.getsecret("SECRET") == blob + + def test_getsecret_keyring(self): + settings = BaseSettings( + {"SECRET": '{"keyring": "my-account"}'}, priority="project" + ) + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = "ring-value" + with mock.patch.dict("sys.modules", {"keyring": mock_keyring}): + assert settings.getsecret("SECRET") == "ring-value" + mock_keyring.get_password.assert_called_once_with("scrapy", "my-account") + + def test_getsecret_keyring_missing_raises(self): + settings = BaseSettings( + {"SECRET": '{"keyring": "nonexistent"}'}, priority="project" + ) + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = None + with ( + mock.patch.dict("sys.modules", {"keyring": mock_keyring}), + pytest.raises(KeyError), + ): + settings.getsecret("SECRET") + def test_getpriority(self): settings = BaseSettings({"key": "value"}, priority=99) assert settings.getpriority("key") == 99 diff --git a/tests/test_utils_secrets.py b/tests/test_utils_secrets.py new file mode 100644 index 000000000..ee5bdced8 --- /dev/null +++ b/tests/test_utils_secrets.py @@ -0,0 +1,263 @@ +from __future__ import annotations + +import json +import logging +from unittest import mock + +import pytest + +from scrapy.utils.secrets import _load_dotenv, resolve_secret + + +class TestResolveSecretPlainValues: + def test_plain_string(self): + assert resolve_secret("my-api-key") == "my-api-key" + + def test_empty_string(self): + assert resolve_secret("") == "" + + def test_integer(self): + assert resolve_secret(42) == "42" + + def test_float(self): + assert resolve_secret(3.14) == "3.14" + + def test_bool(self): + assert resolve_secret(True) == "True" + + def test_json_string_value(self): + # A JSON-encoded string — returned as-is (the original raw string). + assert resolve_secret('"json-encoded-string"') == '"json-encoded-string"' + + def test_json_number_value(self): + assert resolve_secret("123") == "123" + + def test_json_list_value(self): + assert resolve_secret('["a", "b"]') == '["a", "b"]' + + def test_invalid_json(self): + assert resolve_secret("not {json}") == "not {json}" + + def test_json_object_no_recognised_key(self): + # A JSON object that is a literal secret (e.g. a GCP service-account + # JSON blob) — returned as the original raw string. + blob = '{"type": "service_account", "project_id": "my-project"}' + assert resolve_secret(blob) == blob + + def test_dict_no_recognised_key(self): + # Same but already a Python dict (set programmatically). + spec = {"type": "service_account", "project_id": "my-project"} + # raw is None for dict inputs → returned as None (no raw string to fall back to) + assert resolve_secret(spec) is None + + +class TestResolveSecretRaw: + def test_raw_string(self): + assert resolve_secret('{"raw": "literal-value"}') == "literal-value" + + def test_raw_dict(self): + inner = {"type": "service_account", "env": "would-be-ambiguous"} + value = json.dumps({"raw": inner}) + assert resolve_secret(value) == json.dumps(inner) + + def test_raw_list(self): + value = '{"raw": [1, 2, 3]}' + assert resolve_secret(value) == "[1, 2, 3]" + + def test_raw_number(self): + assert resolve_secret('{"raw": 42}') == "42" + + def test_raw_dict_input(self): + assert resolve_secret({"raw": "secret"}) == "secret" + + def test_raw_dict_inner_dict(self): + inner = {"key": "value"} + assert resolve_secret({"raw": inner}) == json.dumps(inner) + + +class TestResolveSecretEnv: + def test_env_var_set(self): + with mock.patch.dict("os.environ", {"MY_SECRET": "s3cr3t"}): + assert resolve_secret('{"env": "MY_SECRET"}') == "s3cr3t" + + def test_env_var_set_dict_input(self): + with mock.patch.dict("os.environ", {"MY_SECRET": "s3cr3t"}): + assert resolve_secret({"env": "MY_SECRET"}) == "s3cr3t" + + def test_env_var_missing_returns_default_none(self, caplog): + with mock.patch.dict("os.environ", {}, clear=False): + # ensure the var is absent + os_env = { + k: v for k, v in __import__("os").environ.items() if k != "MISSING_VAR" + } + with ( + mock.patch.dict("os.environ", os_env, clear=True), + caplog.at_level(logging.WARNING, logger="scrapy.utils.secrets"), + ): + result = resolve_secret('{"env": "MISSING_VAR"}') + assert result is None + assert "MISSING_VAR" in caplog.text + + def test_env_var_missing_returns_explicit_default(self, caplog): + with ( + mock.patch.dict("os.environ", {}, clear=True), + caplog.at_level(logging.WARNING, logger="scrapy.utils.secrets"), + ): + result = resolve_secret('{"env": "MISSING_VAR"}', default="fallback") + assert result == "fallback" + + def test_env_var_missing_logs_warning(self, caplog): + with ( + mock.patch.dict("os.environ", {}, clear=True), + caplog.at_level(logging.WARNING, logger="scrapy.utils.secrets"), + ): + resolve_secret('{"env": "ABSENT_VAR"}') + assert "ABSENT_VAR" in caplog.text + assert caplog.records[0].levelno == logging.WARNING + + def test_env_var_name_not_string_raises(self): + with pytest.raises(ValueError, match='"env" must be a string'): + resolve_secret({"env": 123}) + + +class TestResolveSecretKeyring: + def test_keyring_string_syntax(self): + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = "ring-secret" + with mock.patch.dict("sys.modules", {"keyring": mock_keyring}): + result = resolve_secret('{"keyring": "my-account"}') + assert result == "ring-secret" + mock_keyring.get_password.assert_called_once_with("scrapy", "my-account") + + def test_keyring_dict_syntax_defaults(self): + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = "ring-secret" + with mock.patch.dict("sys.modules", {"keyring": mock_keyring}): + result = resolve_secret('{"keyring": {"username": "my-account"}}') + assert result == "ring-secret" + mock_keyring.get_password.assert_called_once_with("scrapy", "my-account") + + def test_keyring_dict_syntax_custom_service(self): + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = "ring-secret" + with mock.patch.dict("sys.modules", {"keyring": mock_keyring}): + result = resolve_secret( + '{"keyring": {"username": "user", "service": "myapp"}}' + ) + assert result == "ring-secret" + mock_keyring.get_password.assert_called_once_with("myapp", "user") + + def test_keyring_dict_syntax_missing_username_raises(self): + mock_keyring = mock.MagicMock() + with ( + mock.patch.dict("sys.modules", {"keyring": mock_keyring}), + pytest.raises((KeyError, ValueError)), + ): + resolve_secret('{"keyring": {"service": "myapp"}}') + + def test_keyring_entry_missing_raises(self): + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = None + with ( + mock.patch.dict("sys.modules", {"keyring": mock_keyring}), + pytest.raises(KeyError, match="scrapy"), + ): + resolve_secret('{"keyring": "nonexistent-account"}') + + def test_keyring_not_installed_raises_import_error(self): + with ( + mock.patch.dict("sys.modules", {"keyring": None}), + pytest.raises(ImportError, match="keyring"), + ): + resolve_secret('{"keyring": "my-account"}') + + def test_keyring_dict_input(self): + mock_keyring = mock.MagicMock() + mock_keyring.get_password.return_value = "ring-secret" + with mock.patch.dict("sys.modules", {"keyring": mock_keyring}): + result = resolve_secret({"keyring": "my-account"}) + assert result == "ring-secret" + + def test_keyring_invalid_type_raises(self): + mock_keyring = mock.MagicMock() + with ( + mock.patch.dict("sys.modules", {"keyring": mock_keyring}), + pytest.raises(ValueError, match='"keyring" must be'), + ): + resolve_secret({"keyring": 42}) + + def test_keyring_custom_backend(self): + mock_backend_cls = mock.MagicMock() + mock_backend_instance = mock_backend_cls.return_value + mock_backend_instance.get_password.return_value = "backend-secret" + + mock_keyring = mock.MagicMock() + + with ( + mock.patch.dict("sys.modules", {"keyring": mock_keyring}), + mock.patch( + "scrapy.utils.secrets.load_object", return_value=mock_backend_cls + ), + ): + result = resolve_secret( + { + "keyring": { + "username": "user", + "service": "svc", + "backend": "my.module.BackendClass", + } + } + ) + assert result == "backend-secret" + mock_backend_instance.get_password.assert_called_once_with("svc", "user") + + +class TestLoadDotenv: + def test_missing_dotenv_package_warns(self, tmp_path): + dotenv_file = tmp_path / ".env" + dotenv_file.write_text("FOO=bar\n") + with ( + mock.patch.dict("sys.modules", {"dotenv": None}), + pytest.warns(UserWarning, match="python-dotenv"), + ): + _load_dotenv(path=str(dotenv_file)) + + def test_nonexistent_file_is_noop(self, tmp_path): + # A missing .env file is silently skipped regardless of whether + # python-dotenv is installed. + absent = str(tmp_path / "nonexistent.env") + before = dict(__import__("os").environ) + _load_dotenv(path=absent) + assert dict(__import__("os").environ) == before + + def test_loads_variables(self, tmp_path): + pytest.importorskip("dotenv") + dotenv_file = tmp_path / ".env" + dotenv_file.write_text("SCRAPY_TEST_SECRET_VAR=hello\n") + with mock.patch.dict("os.environ", {}, clear=False): + # Remove test var if present + __import__("os").environ.pop("SCRAPY_TEST_SECRET_VAR", None) + _load_dotenv(path=str(dotenv_file)) + assert __import__("os").environ.get("SCRAPY_TEST_SECRET_VAR") == "hello" + __import__("os").environ.pop("SCRAPY_TEST_SECRET_VAR", None) + + def test_real_env_wins_over_dotenv(self, tmp_path): + pytest.importorskip("dotenv") + dotenv_file = tmp_path / ".env" + dotenv_file.write_text("SCRAPY_TEST_OVERRIDE_VAR=from-file\n") + with mock.patch.dict( + "os.environ", {"SCRAPY_TEST_OVERRIDE_VAR": "from-env"}, clear=False + ): + _load_dotenv(path=str(dotenv_file), override=False) + assert __import__("os").environ["SCRAPY_TEST_OVERRIDE_VAR"] == "from-env" + + def test_override_true_replaces_env(self, tmp_path): + pytest.importorskip("dotenv") + dotenv_file = tmp_path / ".env" + dotenv_file.write_text("SCRAPY_TEST_OVERRIDE_VAR=from-file\n") + with mock.patch.dict( + "os.environ", {"SCRAPY_TEST_OVERRIDE_VAR": "from-env"}, clear=False + ): + _load_dotenv(path=str(dotenv_file), override=True) + assert __import__("os").environ["SCRAPY_TEST_OVERRIDE_VAR"] == "from-file" + __import__("os").environ.pop("SCRAPY_TEST_OVERRIDE_VAR", None) From 2021b2c12a3f4acf6fd8b20f9c3dc42837e806f3 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Wed, 10 Jun 2026 21:12:27 +0200 Subject: [PATCH 2/4] Solve typing issues, improve coverage --- conftest.py | 2 ++ docs/topics/settings.rst | 4 +-- pyproject.toml | 2 ++ scrapy/core/downloader/handlers/s3.py | 4 ++- scrapy/pipelines/files.py | 6 ++-- tests/test_utils_secrets.py | 50 +++++++++++++++++++++++++-- tox.ini | 6 ++++ 7 files changed, 65 insertions(+), 9 deletions(-) diff --git a/conftest.py b/conftest.py index cf0568111..e65301e3a 100644 --- a/conftest.py +++ b/conftest.py @@ -149,6 +149,8 @@ def pytest_runtest_setup(item): "botocore", "boto3", "mitmproxy", + "keyring", + "dotenv", ] for module in optional_deps: diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 1c141f489..b5acbe6ab 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -263,7 +263,7 @@ JSON object with an ``"env"`` key: If the environment variable is not set, ``getsecret()`` logs a warning and the default setting value is used instead. -To read the secret from the system keyring (requires the keyring_ package), +To read the secret from the system keyring (requires keyring_ >= 21.7.0), use a ``"keyring"`` key. The value can be a username string — in which case the service name defaults to ``"scrapy"`` — or an object for full control: @@ -813,7 +813,7 @@ each crawl (via :class:`~scrapy.crawler.Crawler`). Relative paths are resolved from the current working directory. If the file does not exist the setting is silently ignored. -Loading a :file:`.env` file requires the python-dotenv_ package:: +Loading a :file:`.env` file requires python-dotenv_ >= 0.10.2:: pip install python-dotenv diff --git a/pyproject.toml b/pyproject.toml index 2d857dc6d..a4072d523 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -269,6 +269,8 @@ markers = [ "requires_botocore: marks tests that need botocore (but not boto3)", "requires_boto3: marks tests that need botocore and boto3", "requires_mitmproxy: marks tests that need mitmproxy", + "requires_keyring: marks tests that need keyring", + "requires_dotenv: marks tests that need python-dotenv", "requires_internet: marks tests that need real Internet access", ] filterwarnings = [ diff --git a/scrapy/core/downloader/handlers/s3.py b/scrapy/core/downloader/handlers/s3.py index a1ede0cfd..927b6bf0b 100644 --- a/scrapy/core/downloader/handlers/s3.py +++ b/scrapy/core/downloader/handlers/s3.py @@ -37,7 +37,9 @@ class S3DownloadHandler(BaseDownloadHandler): # botocore.auth.BaseSigner doesn't have an __init__() with args, only subclasses do self._signer = SignerCls( # type: ignore[call-arg] botocore.credentials.Credentials( - aws_access_key_id, aws_secret_access_key, aws_session_token + aws_access_key_id, + aws_secret_access_key, + aws_session_token, # type: ignore[arg-type] ) ) diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index f856c3555..800288242 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -153,9 +153,9 @@ class FSFilesStore: class S3FilesStore: - AWS_ACCESS_KEY_ID = None - AWS_SECRET_ACCESS_KEY = None - AWS_SESSION_TOKEN = None + AWS_ACCESS_KEY_ID: str | None = None + AWS_SECRET_ACCESS_KEY: str | None = None + AWS_SESSION_TOKEN: str | None = None AWS_ENDPOINT_URL = None AWS_REGION_NAME = None AWS_USE_SSL = None diff --git a/tests/test_utils_secrets.py b/tests/test_utils_secrets.py index ee5bdced8..639738f81 100644 --- a/tests/test_utils_secrets.py +++ b/tests/test_utils_secrets.py @@ -230,8 +230,8 @@ class TestLoadDotenv: _load_dotenv(path=absent) assert dict(__import__("os").environ) == before + @pytest.mark.requires_dotenv def test_loads_variables(self, tmp_path): - pytest.importorskip("dotenv") dotenv_file = tmp_path / ".env" dotenv_file.write_text("SCRAPY_TEST_SECRET_VAR=hello\n") with mock.patch.dict("os.environ", {}, clear=False): @@ -241,8 +241,8 @@ class TestLoadDotenv: assert __import__("os").environ.get("SCRAPY_TEST_SECRET_VAR") == "hello" __import__("os").environ.pop("SCRAPY_TEST_SECRET_VAR", None) + @pytest.mark.requires_dotenv def test_real_env_wins_over_dotenv(self, tmp_path): - pytest.importorskip("dotenv") dotenv_file = tmp_path / ".env" dotenv_file.write_text("SCRAPY_TEST_OVERRIDE_VAR=from-file\n") with mock.patch.dict( @@ -251,8 +251,8 @@ class TestLoadDotenv: _load_dotenv(path=str(dotenv_file), override=False) assert __import__("os").environ["SCRAPY_TEST_OVERRIDE_VAR"] == "from-env" + @pytest.mark.requires_dotenv def test_override_true_replaces_env(self, tmp_path): - pytest.importorskip("dotenv") dotenv_file = tmp_path / ".env" dotenv_file.write_text("SCRAPY_TEST_OVERRIDE_VAR=from-file\n") with mock.patch.dict( @@ -261,3 +261,47 @@ class TestLoadDotenv: _load_dotenv(path=str(dotenv_file), override=True) assert __import__("os").environ["SCRAPY_TEST_OVERRIDE_VAR"] == "from-file" __import__("os").environ.pop("SCRAPY_TEST_OVERRIDE_VAR", None) + + +@pytest.mark.requires_keyring +class TestResolveSecretKeyringIntegration: + """Integration tests that exercise the real keyring library.""" + + def test_get_password_returns_value(self): + import keyring # noqa: PLC0415 + import keyring.backend # noqa: PLC0415 + import keyring.backends.null # noqa: PLC0415 + + class _InMemoryKeyring(keyring.backend.KeyringBackend): + priority = 1 + _store: dict[tuple[str, str], str] = {} + + def get_password(self, service, username): + return self._store.get((service, username)) + + def set_password(self, service, username, password): + self._store[(service, username)] = password + + def delete_password(self, service, username): + self._store.pop((service, username), None) + + backend = _InMemoryKeyring() + backend.set_password("scrapy", "my-account", "real-secret") + original = keyring.get_keyring() + try: + keyring.set_keyring(backend) + assert resolve_secret({"keyring": "my-account"}) == "real-secret" + finally: + keyring.set_keyring(original) + + def test_missing_entry_raises_key_error(self): + import keyring # noqa: PLC0415 + import keyring.backends.null # noqa: PLC0415 + + original = keyring.get_keyring() + try: + keyring.set_keyring(keyring.backends.null.Keyring()) + with pytest.raises(KeyError): + resolve_secret({"keyring": "nonexistent"}) + finally: + keyring.set_keyring(original) diff --git a/tox.ini b/tox.ini index e21e1f6cf..cc1b9dc95 100644 --- a/tox.ini +++ b/tox.ini @@ -47,6 +47,8 @@ deps = mypy==2.1.0 typing-extensions==4.15.0 Pillow==12.2.0 + keyring==22.3.0 + python-dotenv==0.10.2 Protego==0.6.0 Twisted==26.4.0 attrs==26.1.0 @@ -143,6 +145,8 @@ basepython = python3 deps = {[testenv]deps} Pillow + keyring + python-dotenv Twisted[http2] boto3 bpython # optional for shell wrapper tests @@ -160,6 +164,8 @@ basepython = {[pinned]basepython} deps = {[pinned]deps} Pillow==8.3.2 + keyring==21.7.0 + python-dotenv==0.10.2 Twisted[http2]==21.7.0 boto3==1.20.0 bpython==0.7.1 From 1d63fe0941bc8857267a9b314d12df9132e89f4c Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Wed, 10 Jun 2026 21:21:57 +0200 Subject: [PATCH 3/4] Address remaining typing issues --- scrapy/core/downloader/handlers/s3.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scrapy/core/downloader/handlers/s3.py b/scrapy/core/downloader/handlers/s3.py index 927b6bf0b..40be45e9a 100644 --- a/scrapy/core/downloader/handlers/s3.py +++ b/scrapy/core/downloader/handlers/s3.py @@ -37,9 +37,9 @@ class S3DownloadHandler(BaseDownloadHandler): # botocore.auth.BaseSigner doesn't have an __init__() with args, only subclasses do self._signer = SignerCls( # type: ignore[call-arg] botocore.credentials.Credentials( - aws_access_key_id, - aws_secret_access_key, - aws_session_token, # type: ignore[arg-type] + aws_access_key_id or "", + aws_secret_access_key or "", + aws_session_token, ) ) From 2d8467cece4216fccea663a24c3b4f916a4fa3f1 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Wed, 17 Jun 2026 11:59:55 +0200 Subject: [PATCH 4/4] Remove pointless alias --- scrapy/utils/secrets.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scrapy/utils/secrets.py b/scrapy/utils/secrets.py index 7329cf9aa..9ee07b16f 100644 --- a/scrapy/utils/secrets.py +++ b/scrapy/utils/secrets.py @@ -141,7 +141,7 @@ def _load_dotenv(path: str = ".env", override: bool = False) -> None: if not Path(path).is_file(): return try: - from dotenv import load_dotenv as _load # noqa: PLC0415 + from dotenv import load_dotenv # noqa: PLC0415 except ImportError: warnings.warn( "Install the 'python-dotenv' package to load a .env file: " @@ -150,4 +150,4 @@ def _load_dotenv(path: str = ".env", override: bool = False) -> None: stacklevel=2, ) return - _load(dotenv_path=path, override=override) + load_dotenv(dotenv_path=path, override=override)