fix(automatic-exchange-rates): don't follow redirects or log the key in the FXMacroData provider

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")

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)