diff --git a/morpc/req.py b/morpc/req.py index 4fed0d0..e2d22e3 100644 --- a/morpc/req.py +++ b/morpc/req.py @@ -1,4 +1,5 @@ import logging +import re from os import PathLike from time import sleep from httpx import head @@ -7,6 +8,23 @@ logger = logging.getLogger(__name__) +# Query parameters that carry credentials, e.g. the Census API key or an ArcGIS token. +SENSITIVE_PARAMS = ('key', 'token', 'api_key') +_SENSITIVE_PATTERN = re.compile(r'([?&](?:' + '|'.join(SENSITIVE_PARAMS) + r')=)[^&#]*', re.IGNORECASE) + + +def redact(value): + """Return a URL or parameter dict with credential values replaced by "REDACTED", for logs and messages. + + Logs and exception messages end up in committed notebook outputs, so they must never contain API keys. + """ + if value is None: + return None + if isinstance(value, dict): + return {k: ('REDACTED' if str(k).lower() in SENSITIVE_PARAMS else v) for k, v in value.items()} + return _SENSITIVE_PATTERN.sub(r'\1REDACTED', str(value)) + + default_headers = {"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36"} @@ -16,11 +34,11 @@ def get_text_safely(url, params=None, headers=default_headers, session: Session if not isinstance(session, Session): session = Session() - logger.debug(f"Getting data from {url} with parameters {params}.") + logger.debug(f"Getting data from {redact(url)} with parameters {redact(params)}.") r = session.get(url, headers=headers, params=params) if r.status_code != 200: - logger.error(f"Request content: {r.url}") - raise requests.HTTPError + logger.error(f"Request failed with status {r.status_code}: {redact(r.url)}") + raise requests.HTTPError(f"Request failed with status {r.status_code}: {redact(r.url)}", response=r) else: logger.debug(f"Request successful. Returning plain text.") @@ -36,7 +54,7 @@ def get_json_safely(url, params=None, headers=default_headers, session: Session if not isinstance(session, Session): session = Session() - logger.debug(f"Getting data from {url} with parameters {params}.") + logger.debug(f"Getting data from {redact(url)} with parameters {redact(params)}.") r = session.get(url, params=params, headers=headers) if r.status_code != 200: if "Output format not supported" in r.text: @@ -53,10 +71,10 @@ def get_json_safely(url, params=None, headers=default_headers, session: Session else: # 204 means the request was valid but there is nothing to return, which callers may treat as no rows. if r.status_code == 204: - logger.warning(f"No content returned: {r.url}") + logger.warning(f"No content returned: {redact(r.url)}") else: logger.error(f"Request failed. Content: {r.content}") - raise HTTPError(f"Request failed with status {r.status_code}: {r.url}", response=r) + raise HTTPError(f"Request failed with status {r.status_code}: {redact(r.url)}", response=r) else: logger.debug(f"Request successful. Decoding return JSON.") try: @@ -68,12 +86,12 @@ def get_json_safely(url, params=None, headers=default_headers, session: Session r=session.get(url=url, params=params, headers=headers) json = r.json() except Exception as e: - logger.error(f"Request failed: {r.url}") + logger.error(f"Request failed: {redact(r.url)}") logger.error(f"Failed second attempt. {e}") raise RuntimeError logger.error(f"Server returned error {json['error']}") except: - logger.error(f"JSONDecoderError. Check the url. {r.url}") + logger.error(f"JSONDecoderError. Check the url. {redact(r.url)}") raise requests.JSONDecodeError if returnurl: @@ -91,9 +109,13 @@ def get_file_safely(url, output_dir: str | PathLike, chunk_size:int=4096, params filename = os.path.basename(url) filepath = os.path.join(output_dir, filename) - logger.debug(f"Getting file from {url} with parameters {params}.") + logger.debug(f"Getting file from {redact(url)} with parameters {redact(params)}.") with session.get(url, params=params, headers=headers, stream=True) as r: - r.raise_for_status() + try: + r.raise_for_status() + except HTTPError as e: + # requests puts the full URL, including any credentials, in the message. + raise HTTPError(redact(str(e)), response=e.response) from None with open(filepath, "wb") as file: for chunk in r.iter_content(chunk_size=chunk_size): file.write(chunk) @@ -104,7 +126,7 @@ def get_file_safely(url, output_dir: str | PathLike, chunk_size:int=4096, params def post_safely(url, params=None, headers=None): import requests - logger.info(f"Posting data to {url} with parameters {params}.") + logger.info(f"Posting data to {redact(url)} with parameters {redact(params)}.") r = requests.post(url, headers=headers, params=params) if r.status_code != 201: logger.error(f"Request content: {r.content}") @@ -114,7 +136,7 @@ def post_safely(url, params=None, headers=None): try: json = r.json() except: - logger.error(f"JSONDecoderError. Check the url. {r.url}") + logger.error(f"JSONDecoderError. Check the url. {redact(r.url)}") raise requests.JSONDecodeError r.close() @@ -123,7 +145,7 @@ def post_safely(url, params=None, headers=None): def delete_safely(url, params=None, headers=None): import requests - logger.info(f"Deleting data at {url} with parameters {params}.") + logger.info(f"Deleting data at {redact(url)} with parameters {redact(params)}.") r = requests.post(url, headers=headers, params=params) if r.status_code != 204: logger.error(f"Request content: {r.content}") diff --git a/morpc/rest_api/rest_api.py b/morpc/rest_api/rest_api.py index d58d6d5..bdda8e2 100644 --- a/morpc/rest_api/rest_api.py +++ b/morpc/rest_api/rest_api.py @@ -15,7 +15,7 @@ from frictionless.dialect import Control from requests import HTTPError -from morpc.req import get_json_safely +from morpc.req import get_json_safely, redact logger = logging.getLogger(__name__) @@ -301,7 +301,7 @@ def _max_record_count(url): logger.error("Could not find maxRecordCount in response.") raise except KeyError: - logger.error(f"maxRecordCount not in response: {fetched_url}") + logger.error(f"maxRecordCount not in response: {redact(fetched_url)}") raise logger.info(f"Max record count: {max_record_count}") diff --git a/tests/test_req.py b/tests/test_req.py index 8c43c41..8c1cf3d 100644 --- a/tests/test_req.py +++ b/tests/test_req.py @@ -41,3 +41,89 @@ def test_get_json_safely_http_error_carries_response(): def test_get_json_safely_returns_json_on_success(): assert get_json_safely("http://example", session=_FakeSession(_Response(200, [["a"], ["1"]]))) == [["a"], ["1"]] + + +# Census and ArcGIS requests carry credentials as query parameters (key=, token=). Logs and exception +# messages end up in committed notebook outputs, so they must never contain them. + +SECRET = "s3cr3t-api-key" +SECRET_URL = f"http://example/data?get=NAME&key={SECRET}" + + +class _SecretResponse(_Response): + def __init__(self, status_code, payload=None): + super().__init__(status_code, payload) + self.url = SECRET_URL + + def raise_for_status(self): + # Mirrors requests: the message includes the full URL. + if self.status_code != 200: + raise HTTPError(f"{self.status_code} Client Error: Not Found for url: {self.url}", response=self) + + def iter_content(self, chunk_size=1): + return iter([b"data"]) + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + +class _SecretSession(_FakeSession): + def get(self, url, params=None, headers=None, stream=False): + return self._response + + +def _assert_no_secret(caplog, excinfo=None): + assert SECRET not in caplog.text + if excinfo is not None: + assert SECRET not in str(excinfo.value) + + +@pytest.mark.parametrize("status", [204, 400, 404]) +def test_get_json_safely_failure_does_not_leak_key(caplog, status): + caplog.set_level("DEBUG") + with pytest.raises(HTTPError) as excinfo: + get_json_safely("http://example/data", params={"key": SECRET}, session=_SecretSession(_SecretResponse(status))) + _assert_no_secret(caplog, excinfo) + + +def test_get_json_safely_success_does_not_leak_key(caplog): + caplog.set_level("DEBUG") + get_json_safely("http://example/data", params={"key": SECRET}, session=_SecretSession(_SecretResponse(200, [["a"]]))) + _assert_no_secret(caplog) + + +def test_get_json_safely_decode_error_does_not_leak_key(caplog): + class _BadJSON(_SecretResponse): + def json(self): + raise ValueError("not json") + caplog.set_level("DEBUG") + with pytest.raises(Exception) as excinfo: + get_json_safely(SECRET_URL, session=_SecretSession(_BadJSON(200))) + _assert_no_secret(caplog, excinfo) + + +def test_get_text_safely_failure_does_not_leak_key(caplog): + from morpc.req import get_text_safely + caplog.set_level("DEBUG") + with pytest.raises(HTTPError) as excinfo: + get_text_safely(SECRET_URL, session=_SecretSession(_SecretResponse(400))) + _assert_no_secret(caplog, excinfo) + + +def test_get_file_safely_failure_does_not_leak_key(caplog, tmp_path): + from morpc.req import get_file_safely + caplog.set_level("DEBUG") + with pytest.raises(HTTPError) as excinfo: + get_file_safely("http://example/data.csv", tmp_path, params={"token": SECRET}, session=_SecretSession(_SecretResponse(404))) + _assert_no_secret(caplog, excinfo) + + +def test_redact_hides_credentials_in_urls_and_params(): + from morpc.req import redact + assert SECRET not in redact(f"http://x/y?a=1&KEY={SECRET}&token={SECRET}&api_key={SECRET}") + assert redact(f"http://x/y?a=1&key={SECRET}") == "http://x/y?a=1&key=REDACTED" + assert redact({"get": "NAME", "key": SECRET}) == {"get": "NAME", "key": "REDACTED"} + assert redact(None) is None