From 237f8efc6c1c606803b6b6658025d49b9dc4350d Mon Sep 17 00:00:00 2001 From: Robert Tidball Date: Mon, 5 Oct 2026 21:45:20 +1100 Subject: [PATCH] fix(automatic-exchange-rates): don't follow redirects or log the key in the FXMacroData provider --- .../currencies/exchange_rates/providers.py | 42 +++++++- .../tests/test_fxmacrodata_provider.py | 97 ++++++++++++++++++- 2 files changed, 135 insertions(+), 4 deletions(-) diff --git a/app/apps/currencies/exchange_rates/providers.py b/app/apps/currencies/exchange_rates/providers.py index e9275158..4d79d08c 100644 --- a/app/apps/currencies/exchange_rates/providers.py +++ b/app/apps/currencies/exchange_rates/providers.py @@ -591,6 +591,7 @@ class FXMacroDataProvider(ExchangeRateProvider): rates_inverted = False # /forex/EUR/USD returns how many USD one EUR buys def __init__(self, api_key: str): + api_key = (api_key or "").strip() super().__init__(api_key) self.session = requests.Session() self.session.headers.update({"X-API-Key": api_key}) @@ -604,6 +605,14 @@ class FXMacroDataProvider(ExchangeRateProvider): ) -> List[Tuple[Currency, Currency, Decimal]]: results = [] + # requests puts the header value in its InvalidHeader message, which would + # then end up in the logs, so reject a bad key before making any request + if any(char.isspace() or not char.isprintable() for char in self.api_key): + logger.error( + "FXMacroData API key contains whitespace or control characters" + ) + return results + for target_currency in target_currencies: if target_currency.exchange_currency not in exchange_currencies: continue @@ -617,14 +626,41 @@ class FXMacroDataProvider(ExchangeRateProvider): pair = f"{base_currency.code}/{target_currency.code}" try: - response = self.session.get(f"{self.BASE_URL}/{pair}") + # requests only drops Authorization on a cross-host redirect, so + # following one would send X-API-Key to the new host + response = self.session.get( + f"{self.BASE_URL}/{pair}", allow_redirects=False + ) + if 300 <= response.status_code < 400: + logger.error( + f"FXMacroData redirected the request for {pair} " + f"(HTTP {response.status_code}), not following it" + ) + continue response.raise_for_status() + try: + payload = response.json() + except ValueError: + logger.error(f"FXMacroData returned a non-JSON response for {pair}") + continue + + if not isinstance(payload, dict): + payload = {} + data = payload.get("data") + if not isinstance(data, list): + detail = payload.get("detail") + logger.error( + f"Unexpected response structure from FXMacroData for {pair}" + + (f": {detail}" if isinstance(detail, str) else "") + ) + continue + # val is null on dates without a fixing, so use the newest non-null row rows = [ row - for row in response.json().get("data", []) - if row.get("val") is not None + for row in data + if isinstance(row, dict) and row.get("val") is not None ] if not rows: logger.error(f"No rate returned for {pair} from FXMacroData") diff --git a/app/apps/currencies/tests/test_fxmacrodata_provider.py b/app/apps/currencies/tests/test_fxmacrodata_provider.py index d4893668..0a376512 100644 --- a/app/apps/currencies/tests/test_fxmacrodata_provider.py +++ b/app/apps/currencies/tests/test_fxmacrodata_provider.py @@ -1,4 +1,6 @@ +import threading from decimal import Decimal +from http.server import BaseHTTPRequestHandler, HTTPServer from unittest import TestCase from unittest.mock import MagicMock, patch @@ -17,10 +19,25 @@ class _FakeCurrency: def _response(payload): response = MagicMock() + response.status_code = 200 response.json.return_value = payload return response +class _RedirectHandler(BaseHTTPRequestHandler): + requested = [] + + def do_GET(self): + self.requested.append(self.path) + self.send_response(302) + self.send_header("Location", "/elsewhere") + self.send_header("Content-Length", "0") + self.end_headers() + + def log_message(self, *args): + pass + + class FXMacroDataProviderTests(TestCase): def setUp(self): self.usd = _FakeCurrency("USD") @@ -44,7 +61,9 @@ class FXMacroDataProviderTests(TestCase): ) as mock_get: rates = self.provider.get_rates([self.eur], {self.usd}) - mock_get.assert_called_once_with("https://api.fxmacrodata.com/v1/forex/USD/EUR") + mock_get.assert_called_once_with( + "https://api.fxmacrodata.com/v1/forex/USD/EUR", allow_redirects=False + ) self.assertEqual(rates, [(self.usd, self.eur, Decimal("0.8542"))]) def test_skips_pair_without_rates(self): @@ -67,6 +86,82 @@ class FXMacroDataProviderTests(TestCase): self.assertEqual(rates, []) + def test_does_not_follow_redirects(self): + _RedirectHandler.requested = [] + server = HTTPServer(("127.0.0.1", 0), _RedirectHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + self.addCleanup(server.server_close) + self.addCleanup(server.shutdown) + base_url = f"http://127.0.0.1:{server.server_port}/v1/forex" + + with patch.object(FXMacroDataProvider, "BASE_URL", base_url): + with self.assertLogs( + "apps.currencies.exchange_rates.providers", "ERROR" + ) as logs: + rates = self.provider.get_rates([self.eur], {self.usd}) + + self.assertEqual(rates, []) + self.assertEqual(_RedirectHandler.requested, ["/v1/forex/USD/EUR"]) + self.assertIn("redirected", logs.output[0]) + self.assertNotIn("test-key", "".join(logs.output)) + + def test_strips_whitespace_around_api_key(self): + provider = FXMacroDataProvider(" test-key\n") + + self.assertEqual(provider.session.headers["X-API-Key"], "test-key") + + def test_rejects_malformed_api_key_without_logging_it(self): + for api_key in ("test-key\r\nX-Injected: 1", "test key", "test-key\x00"): + provider = FXMacroDataProvider(api_key) + + with patch.object(provider.session, "get") as mock_get: + with self.assertLogs( + "apps.currencies.exchange_rates.providers", "ERROR" + ) as logs: + rates = provider.get_rates([self.eur], {self.usd}) + + self.assertEqual(rates, []) + mock_get.assert_not_called() + self.assertNotIn("test", "".join(logs.output)) + + def test_logs_api_detail_when_data_is_missing(self): + with patch.object( + self.provider.session, + "get", + return_value=_response({"detail": "Invalid API key"}), + ): + with self.assertLogs( + "apps.currencies.exchange_rates.providers", "ERROR" + ) as logs: + rates = self.provider.get_rates([self.eur], {self.usd}) + + self.assertEqual(rates, []) + self.assertIn("Invalid API key", logs.output[0]) + + def test_skips_pair_on_malformed_body(self): + not_json = _response(None) + not_json.json.side_effect = ValueError("Expecting value") + + for response in (not_json, _response([]), _response({"data": {}})): + with patch.object(self.provider.session, "get", return_value=response): + with self.assertLogs( + "apps.currencies.exchange_rates.providers", "ERROR" + ): + rates = self.provider.get_rates([self.eur], {self.usd}) + + self.assertEqual(rates, []) + + def test_ignores_rows_that_are_not_objects(self): + payload = {"data": ["oops", {"date": "2026-09-25", "val": 0.8542}]} + + with patch.object( + self.provider.session, "get", return_value=_response(payload) + ): + rates = self.provider.get_rates([self.eur], {self.usd}) + + self.assertEqual(rates, [(self.usd, self.eur, Decimal("0.8542"))]) + def test_is_registered_with_an_api_key(self): self.assertTrue(FXMacroDataProvider.requires_api_key()) self.assertIs(PROVIDER_MAPPING["fxmacrodata"], FXMacroDataProvider)