mirror of
https://github.com/TauricResearch/TradingAgents.git
synced 2026-09-19 11:15:24 +03:00
fix(dataflows): keep vendor API keys out of request errors
- FRED and Alpha Vantage authenticate with a query parameter, and requests quotes the full URL in HTTP, connection and timeout errors, so the key was written into any log or traceback that recorded one - route both vendors through one request helper that re-raises a requests error as the same class with the key removed, carrying no request, response or exception chain, each of which still held the URL #1324
This commit is contained in:
@@ -13,9 +13,12 @@ import pytest
|
|||||||
import tradingagents.dataflows.alpha_vantage_common as av
|
import tradingagents.dataflows.alpha_vantage_common as av
|
||||||
import tradingagents.dataflows.alpha_vantage_fundamentals as avf
|
import tradingagents.dataflows.alpha_vantage_fundamentals as avf
|
||||||
import tradingagents.dataflows.alpha_vantage_stock as avs
|
import tradingagents.dataflows.alpha_vantage_stock as avs
|
||||||
|
import tradingagents.dataflows.utils as utils
|
||||||
|
|
||||||
|
|
||||||
class _FakeResponse:
|
class _FakeResponse:
|
||||||
|
status_code = 200
|
||||||
|
|
||||||
def __init__(self, text):
|
def __init__(self, text):
|
||||||
self.text = text
|
self.text = text
|
||||||
|
|
||||||
@@ -34,7 +37,7 @@ def _patched_get(body, capture=None):
|
|||||||
@pytest.mark.unit
|
@pytest.mark.unit
|
||||||
def test_request_passes_timeout(monkeypatch):
|
def test_request_passes_timeout(monkeypatch):
|
||||||
captured = {}
|
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"})
|
av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"})
|
||||||
assert captured.get("timeout") == av.REQUEST_TIMEOUT # #990
|
assert captured.get("timeout") == av.REQUEST_TIMEOUT # #990
|
||||||
|
|
||||||
@@ -42,7 +45,7 @@ def test_request_passes_timeout(monkeypatch):
|
|||||||
@pytest.mark.unit
|
@pytest.mark.unit
|
||||||
def test_rate_limit_detected(monkeypatch):
|
def test_rate_limit_detected(monkeypatch):
|
||||||
body = '{"Information": "Our standard API rate limit is 25 requests per day. ... your API key ..."}'
|
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):
|
with pytest.raises(av.AlphaVantageRateLimitError):
|
||||||
av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"})
|
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).
|
# (transient) rate limit, but surface as a real configuration error (#991).
|
||||||
body = ('{"Information": "the parameter apikey is invalid or missing. '
|
body = ('{"Information": "the parameter apikey is invalid or missing. '
|
||||||
'Please claim your free API key on (https://www.alphavantage.co/support/#api-key)."}')
|
'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):
|
with pytest.raises(av.AlphaVantageNotConfiguredError):
|
||||||
av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"})
|
av._make_api_request("TIME_SERIES_DAILY", {"symbol": "AAPL"})
|
||||||
with pytest.raises(av.AlphaVantageRateLimitError): # sanity: rate-limit path still distinct
|
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"})
|
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):
|
def test_empty_body_still_passes_through(monkeypatch):
|
||||||
monkeypatch.setattr(avs, "_make_api_request", lambda *a, **k: "")
|
monkeypatch.setattr(avs, "_make_api_request", lambda *a, **k: "")
|
||||||
assert avs.get_stock("IBM", "2024-05-09", "2024-05-10") == ""
|
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)
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import unittest
|
|||||||
from unittest import mock
|
from unittest import mock
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
import requests
|
||||||
|
|
||||||
import tradingagents.dataflows.config as config_module
|
import tradingagents.dataflows.config as config_module
|
||||||
import tradingagents.default_config as default_config
|
import tradingagents.default_config as default_config
|
||||||
@@ -233,3 +234,53 @@ class FredRoutingTests(unittest.TestCase):
|
|||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.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
|
||||||
|
|||||||
@@ -4,9 +4,9 @@ from datetime import datetime
|
|||||||
from io import StringIO
|
from io import StringIO
|
||||||
|
|
||||||
import pandas as pd
|
import pandas as pd
|
||||||
import requests
|
|
||||||
|
|
||||||
from .errors import VendorNotConfiguredError, VendorRateLimitError
|
from .errors import VendorNotConfiguredError, VendorRateLimitError
|
||||||
|
from .utils import get_scrubbed
|
||||||
|
|
||||||
API_BASE_URL = "https://www.alphavantage.co/query"
|
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
|
AlphaVantageRateLimitError: When API rate limit is exceeded
|
||||||
"""
|
"""
|
||||||
# Create a copy of params to avoid modifying the original
|
# Create a copy of params to avoid modifying the original
|
||||||
|
api_key = get_api_key()
|
||||||
api_params = params.copy()
|
api_params = params.copy()
|
||||||
api_params.update({
|
api_params.update({
|
||||||
"function": function_name,
|
"function": function_name,
|
||||||
"apikey": get_api_key(),
|
"apikey": api_key,
|
||||||
"source": "trading_agents",
|
"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
|
# Remove entitlement if it's None or empty
|
||||||
api_params.pop("entitlement", None)
|
api_params.pop("entitlement", None)
|
||||||
|
|
||||||
response = requests.get(API_BASE_URL, params=api_params, timeout=REQUEST_TIMEOUT)
|
response = get_scrubbed(
|
||||||
response.raise_for_status()
|
API_BASE_URL, params=api_params, timeout=REQUEST_TIMEOUT, secret=api_key
|
||||||
|
)
|
||||||
|
|
||||||
response_text = response.text
|
response_text = response.text
|
||||||
|
|
||||||
|
|||||||
@@ -13,9 +13,9 @@ import os
|
|||||||
from datetime import datetime, timedelta
|
from datetime import datetime, timedelta
|
||||||
|
|
||||||
import pytz
|
import pytz
|
||||||
import requests
|
|
||||||
|
|
||||||
from .errors import VendorNotConfiguredError
|
from .errors import VendorNotConfiguredError
|
||||||
|
from .utils import get_scrubbed
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
@@ -134,9 +134,13 @@ def _fred_today() -> str:
|
|||||||
|
|
||||||
def _request(path: str, params: dict) -> dict:
|
def _request(path: str, params: dict) -> dict:
|
||||||
"""GET a FRED endpoint, surfacing FRED's JSON error body on a bad request."""
|
"""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"}
|
api_key = get_api_key()
|
||||||
response = requests.get(
|
response = get_scrubbed(
|
||||||
f"{FRED_API_BASE}/{path}", params=api_params, timeout=REQUEST_TIMEOUT
|
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
|
# FRED returns 400 with a JSON {"error_message": ...} for unknown series IDs
|
||||||
# or malformed params; turn that into a clear, actionable error.
|
# or malformed params; turn that into a clear, actionable error.
|
||||||
@@ -146,7 +150,6 @@ def _request(path: str, params: dict) -> dict:
|
|||||||
except ValueError:
|
except ValueError:
|
||||||
message = response.text
|
message = response.text
|
||||||
raise ValueError(f"FRED request failed: {message}")
|
raise ValueError(f"FRED request failed: {message}")
|
||||||
response.raise_for_status()
|
|
||||||
return response.json()
|
return response.json()
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,8 @@
|
|||||||
import re
|
import re
|
||||||
from datetime import date
|
from datetime import date
|
||||||
|
|
||||||
|
import requests
|
||||||
|
|
||||||
# Tickers can contain letters, digits, dot, dash, underscore, caret
|
# Tickers can contain letters, digits, dot, dash, underscore, caret
|
||||||
# (index symbols like ^GSPC), equals (futures like GC=F), and plus
|
# (index symbols like ^GSPC), equals (futures like GC=F), and plus
|
||||||
# (forex/CFD symbols like XAUUSD+). None of these enable directory
|
# (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():
|
def get_current_date():
|
||||||
return date.today().strftime("%Y-%m-%d")
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user