diff --git a/tests/test_alpha_vantage_hardening.py b/tests/test_alpha_vantage_hardening.py index 494f2bac6..130d85fd2 100644 --- a/tests/test_alpha_vantage_hardening.py +++ b/tests/test_alpha_vantage_hardening.py @@ -13,9 +13,12 @@ import pytest import tradingagents.dataflows.alpha_vantage_common as av import tradingagents.dataflows.alpha_vantage_fundamentals as avf import tradingagents.dataflows.alpha_vantage_stock as avs +import tradingagents.dataflows.utils as utils class _FakeResponse: + status_code = 200 + def __init__(self, text): self.text = text @@ -34,7 +37,7 @@ def _patched_get(body, capture=None): @pytest.mark.unit def test_request_passes_timeout(monkeypatch): captured = {} - monkeypatch.setattr(av.requests, "get", _patched_get("Date,Close\n2025-01-02,1.0", captured)) + monkeypatch.setattr(utils.requests, "get", _patched_get("Date,Close\n2025-01-02,1.0", captured)) av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"}) assert captured.get("timeout") == av.REQUEST_TIMEOUT # #990 @@ -42,7 +45,7 @@ def test_request_passes_timeout(monkeypatch): @pytest.mark.unit def test_rate_limit_detected(monkeypatch): body = '{"Information": "Our standard API rate limit is 25 requests per day. ... your API key ..."}' - monkeypatch.setattr(av.requests, "get", _patched_get(body)) + monkeypatch.setattr(utils.requests, "get", _patched_get(body)) with pytest.raises(av.AlphaVantageRateLimitError): av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"}) @@ -53,11 +56,11 @@ def test_invalid_key_not_mislabeled_as_rate_limit(monkeypatch): # (transient) rate limit, but surface as a real configuration error (#991). body = ('{"Information": "the parameter apikey is invalid or missing. ' 'Please claim your free API key on (https://www.alphavantage.co/support/#api-key)."}') - monkeypatch.setattr(av.requests, "get", _patched_get(body)) + monkeypatch.setattr(utils.requests, "get", _patched_get(body)) with pytest.raises(av.AlphaVantageNotConfiguredError): av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"}) with pytest.raises(av.AlphaVantageRateLimitError): # sanity: rate-limit path still distinct - monkeypatch.setattr(av.requests, "get", _patched_get('{"Note": "API call frequency is 5 calls per minute."}')) + monkeypatch.setattr(utils.requests, "get", _patched_get('{"Note": "API call frequency is 5 calls per minute."}')) av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"}) @@ -133,3 +136,18 @@ def test_unparseable_body_is_never_served_untrimmed(monkeypatch): def test_empty_body_still_passes_through(monkeypatch): monkeypatch.setattr(avs, "_make_api_request", lambda *a, **k: "") assert avs.get_stock("IBM", "2024-05-09", "2024-05-10") == "" + + +def test_request_error_message_carries_no_key(monkeypatch): + # Alpha Vantage also sends its key in the URL (#1324). + import requests + key = "AVKEY1234567890XYZ" + monkeypatch.setenv("ALPHA_VANTAGE_API_KEY", key) + + def boom(*a, **k): + raise requests.Timeout(f"Read timed out. url: https://www.alphavantage.co/query?apikey={key}") + + monkeypatch.setattr(utils.requests, "get", boom) + with pytest.raises(requests.Timeout) as caught: + av._make_api_request("OVERVIEW", {"symbol": "IBM"}) + assert key not in str(caught.value) diff --git a/tests/test_fred.py b/tests/test_fred.py index a4ffbd213..a4ec29962 100644 --- a/tests/test_fred.py +++ b/tests/test_fred.py @@ -8,6 +8,7 @@ import unittest from unittest import mock import pytest +import requests import tradingagents.dataflows.config as config_module import tradingagents.default_config as default_config @@ -233,3 +234,53 @@ class FredRoutingTests(unittest.TestCase): if __name__ == "__main__": unittest.main() + + +_KEY = "abcdef0123456789abcdef0123456789" + + +@pytest.mark.unit +class TestKeyKeptOutOfErrors: + """The key travels as a query parameter, and requests quotes the full URL in + its error messages, so any log or traceback would carry it (#1324).""" + + def _raises(self, side_effect): + with mock.patch.dict("os.environ", {"FRED_API_KEY": _KEY}), \ + mock.patch("tradingagents.dataflows.utils.requests.get", side_effect=side_effect), \ + pytest.raises(requests.RequestException) as caught: + fred._request("series", {"series_id": "DGS10"}) + return caught.value + + def test_http_error_message_carries_no_key(self): + response = mock.Mock(status_code=502) + response.raise_for_status.side_effect = requests.HTTPError( + f"502 Server Error for url: https://api.stlouisfed.org/fred/series?api_key={_KEY}", + response=response, + ) + with mock.patch.dict("os.environ", {"FRED_API_KEY": _KEY}), \ + mock.patch("tradingagents.dataflows.utils.requests.get", return_value=response), \ + pytest.raises(requests.HTTPError) as caught: + fred._request("series", {"series_id": "DGS10"}) + exc = caught.value + assert _KEY not in str(exc) and _KEY not in repr(exc) + # The response and request carry the full URL, so they are not attached. + assert exc.response is None and exc.request is None + assert exc.__cause__ is None and exc.__context__ is None # no chain holds the key + + def test_connection_error_before_any_response_carries_no_key(self): + exc = self._raises(requests.ConnectionError( + f"Max retries exceeded with url: /fred/series?series_id=DGS10&api_key={_KEY}")) + assert isinstance(exc, requests.ConnectionError) + assert _KEY not in str(exc) and exc.__context__ is None + + +@pytest.mark.unit +def test_error_without_the_key_in_its_message_still_drops_the_request(): + # Some timeout messages omit the URL, but the attached request still has it. + import requests as rq + req = rq.Request("GET", f"https://api.stlouisfed.org/fred/series?api_key={_KEY}").prepare() + with mock.patch.dict("os.environ", {"FRED_API_KEY": _KEY}), \ + mock.patch("tradingagents.dataflows.utils.requests.get", side_effect=rq.Timeout("Read timed out.", request=req)), \ + pytest.raises(rq.Timeout) as caught: + fred._request("series", {"series_id": "DGS10"}) + assert caught.value.request is None diff --git a/tradingagents/dataflows/alpha_vantage_common.py b/tradingagents/dataflows/alpha_vantage_common.py index ecbe4dcec..95dda491d 100644 --- a/tradingagents/dataflows/alpha_vantage_common.py +++ b/tradingagents/dataflows/alpha_vantage_common.py @@ -4,9 +4,9 @@ from datetime import datetime from io import StringIO import pandas as pd -import requests from .errors import VendorNotConfiguredError, VendorRateLimitError +from .utils import get_scrubbed API_BASE_URL = "https://www.alphavantage.co/query" @@ -66,10 +66,11 @@ def _make_api_request(function_name: str, params: dict) -> dict | str: AlphaVantageRateLimitError: When API rate limit is exceeded """ # Create a copy of params to avoid modifying the original + api_key = get_api_key() api_params = params.copy() api_params.update({ "function": function_name, - "apikey": get_api_key(), + "apikey": api_key, "source": "trading_agents", }) @@ -83,8 +84,9 @@ def _make_api_request(function_name: str, params: dict) -> dict | str: # Remove entitlement if it's None or empty api_params.pop("entitlement", None) - response = requests.get(API_BASE_URL, params=api_params, timeout=REQUEST_TIMEOUT) - response.raise_for_status() + response = get_scrubbed( + API_BASE_URL, params=api_params, timeout=REQUEST_TIMEOUT, secret=api_key + ) response_text = response.text diff --git a/tradingagents/dataflows/fred.py b/tradingagents/dataflows/fred.py index 98f1827b6..28312e434 100644 --- a/tradingagents/dataflows/fred.py +++ b/tradingagents/dataflows/fred.py @@ -13,9 +13,9 @@ import os from datetime import datetime, timedelta import pytz -import requests from .errors import VendorNotConfiguredError +from .utils import get_scrubbed logger = logging.getLogger(__name__) @@ -134,9 +134,13 @@ def _fred_today() -> str: def _request(path: str, params: dict) -> dict: """GET a FRED endpoint, surfacing FRED's JSON error body on a bad request.""" - api_params = {**params, "api_key": get_api_key(), "file_type": "json"} - response = requests.get( - f"{FRED_API_BASE}/{path}", params=api_params, timeout=REQUEST_TIMEOUT + api_key = get_api_key() + response = get_scrubbed( + f"{FRED_API_BASE}/{path}", + params={**params, "api_key": api_key, "file_type": "json"}, + timeout=REQUEST_TIMEOUT, + secret=api_key, + passthrough=(400,), ) # FRED returns 400 with a JSON {"error_message": ...} for unknown series IDs # or malformed params; turn that into a clear, actionable error. @@ -146,7 +150,6 @@ def _request(path: str, params: dict) -> dict: except ValueError: message = response.text raise ValueError(f"FRED request failed: {message}") - response.raise_for_status() return response.json() diff --git a/tradingagents/dataflows/utils.py b/tradingagents/dataflows/utils.py index 71806b47f..39ede36f0 100644 --- a/tradingagents/dataflows/utils.py +++ b/tradingagents/dataflows/utils.py @@ -1,6 +1,8 @@ import re from datetime import date +import requests + # Tickers can contain letters, digits, dot, dash, underscore, caret # (index symbols like ^GSPC), equals (futures like GC=F), and plus # (forex/CFD symbols like XAUUSD+). None of these enable directory @@ -39,3 +41,24 @@ def safe_ticker_component(value: str, *, max_len: int = 32) -> str: def get_current_date(): return date.today().strftime("%Y-%m-%d") + + +def get_scrubbed(url: str, *, params: dict, timeout: float, secret: str, passthrough=()): + """``requests.get`` plus ``raise_for_status``, with ``secret`` kept out of errors. + + Vendors that authenticate with a query parameter put the key in the URL, and + requests quotes the full URL in HTTP, connection and timeout errors, so any + log or traceback that records one would carry the key (#1324). A requests + error is re-raised as the same class with the key replaced and nothing + attached: no request or response (both hold the URL) and no exception chain, + which is why this raises after the ``except`` block rather than inside it. + Statuses in ``passthrough`` are returned for the caller to handle. + """ + try: + response = requests.get(url, params=params, timeout=timeout) + if response.status_code not in passthrough: + response.raise_for_status() + return response + except requests.RequestException as exc: + error = type(exc)(str(exc).replace(secret, "***")) if secret else exc + raise error