diff --git a/packages/flow/openadapt_flow/network_capture.py b/packages/flow/openadapt_flow/network_capture.py index 2a707b9c92..14b1e9f12f 100644 --- a/packages/flow/openadapt_flow/network_capture.py +++ b/packages/flow/openadapt_flow/network_capture.py @@ -75,7 +75,7 @@ def _page_of(request: Any) -> Any: re.I, ) SECRET_NAME = re.compile( - r"pass(word|wd|phrase)?\b|passwd|clearpass|pwd|secret|private[_-]?key|^pin$|^otp$", + r"pass(word|wd|phrase)?\b|passwd|pwd|secret|private[_-]?key|^pin$|^otp$", re.I, ) SECRET = "" diff --git a/packages/flow/openadapt_flow/report_redaction.py b/packages/flow/openadapt_flow/report_redaction.py index 4b1be3b91d..1770cb75b7 100644 --- a/packages/flow/openadapt_flow/report_redaction.py +++ b/packages/flow/openadapt_flow/report_redaction.py @@ -2,16 +2,18 @@ A run report (``report.json``) is a shareable artifact: an offline Seal signs its exact bytes and the signed report can be published as a receipt. URLs the -run saw can carry the application's session: OpenEMR puts its per-session -CSRF token in ``main.php?token_main=...``, OAuth redirects carry ``code`` and +run saw can carry the application's session: an app may put its per-session +CSRF token in a query parameter such as ``main.php?csrf_token=...``, OAuth redirects carry ``code`` and ``state``, and a captured header can hold ``Authorization: Bearer ...`` or a ``Cookie``. None of that belongs in a receipt. :func:`redact_report` walks every string in the report and replaces -* the value of a secret query, fragment or form parameter (``token_main=``, - ``csrf_token_form=``, ``PHPSESSID=``, ``access_token=``, ``code=``, - ``state=``, ``signature=`` and the rest of :data:`DEFAULT_PARAMS`); +* the value of a secret query, fragment or form parameter: one in + :data:`DEFAULT_PARAMS` (``PHPSESSID=``, ``access_token=``, ``code=``, + ``state=``, ``signature=``, ...) or one whose name has a CSRF, XSRF, + token or session-id part (:func:`is_credential_name`: ``csrf_token_form=``, + ``token_main=``, ``auth_token=``); * the value of a credential header (``Authorization``, ``Cookie``, ``Set-Cookie`` and the rest of :data:`DEFAULT_HEADERS`), in a ``Name: value`` line, a JSON key, or a ``{"name": ..., "value": ...}`` pair; @@ -40,7 +42,7 @@ ``keep`` may only name the parameters that are also ordinary words in some apps (:data:`KEEPABLE_PARAMS`, such as ``state`` for a US state field); a -credential name such as ``token_main`` or ``access_token`` is always redacted. +credential name such as ``csrf_token`` or ``access_token`` is always redacted. Nothing here touches the network. """ @@ -65,9 +67,8 @@ #: credential, OAuth grant or request signature. Compared case-insensitively. DEFAULT_PARAMS: Final = frozenset( { - # CSRF and per-session tokens (OpenEMR, Laravel, Rails, Django, ...) - "token_main", - "csrf_token_form", + # CSRF and per-session tokens (Laravel, Rails, Django, ...); any other + # name with such a part is caught by is_credential_name. "csrf_token", "csrf", "_csrf", @@ -128,9 +129,7 @@ #: JSON object keys whose whole string value is a credential, besides headers. _SECRET_KEYS: Final = frozenset( { - "token_main", "csrf_token", - "csrf_token_form", "access_token", "id_token", "refresh_token", @@ -214,6 +213,47 @@ def params_for(self, host: Optional[str]) -> frozenset[str]: DEFAULT_POLICY: Final = RedactionPolicy() +#: Name parts that make a parameter or JSON key a credential in any app. +_CREDENTIAL_PARTS: Final = frozenset( + { + "token", + "csrf", + "xsrf", + "csrftoken", + "xsrftoken", + "sessid", + "sessionid", + } +) +_NAME_PARTS: Final = re.compile(r"[a-z0-9]+") +_CAMEL_BOUNDARY: Final = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") + + +def is_credential_name(name: str, *, header: bool = False) -> bool: + """Whether a parameter, key or header name says its value is a credential. + + The name is split into parts on anything but letters and digits; a part + that is a token, CSRF/XSRF token or session id makes it one + (``csrf_token_form``, ``X-Frappe-CSRF-Token``, ``app_sessid``). For a + header (``header=True``) an ``authorization`` part does too + (``Espo-Authorization``); elsewhere that word names an approval, not a + credential. A parameter or header named so is redacted in every app and + cannot be kept. + """ + # camelCase splits too: sessionToken, accessToken, RequestVerificationToken. + spaced = _CAMEL_BOUNDARY.sub(" ", name) + parts = _NAME_PARTS.findall(spaced.lower()) + # A session id header spelled as two parts (Session-Id, x-session-id). + # Only headers: a report's own fields such as dispatch_session_id name a + # run's internal session, not a credential. + joined = {a + b for a, b in zip(parts, parts[1:])} if header else set() + return any( + part in _CREDENTIAL_PARTS + or part.startswith(("csrf", "xsrf")) + or (header and part == "authorization") + for part in [*parts, *joined] + ) + @dataclass(frozen=True) class Redaction: @@ -349,7 +389,9 @@ def _redact_params(text: str, policy: RedactionPolicy, reasons: list[str]) -> st if match.start() < cursor: continue # inside a value already redacted name = match.group("name").lower() - if name not in policy.params_for(_host_at(text, match.start())): + if name not in policy.params_for( + _host_at(text, match.start()) + ) and not is_credential_name(match.group("name")): continue value = _PARAM_VALUE.match(text, match.end()) if value is None: @@ -370,7 +412,9 @@ def redact_text( def header(match: re.Match[str]) -> str: name = match.group("name") - if name.lower() not in policy.headers: + if name.lower() not in policy.headers and not is_credential_name( + name, header=True + ): return match.group(0) new = _header_value(name, match.group("value")) if new == match.group("value") or new.strip() == match.group("value").strip(): @@ -415,7 +459,10 @@ def _redact_node( pair_header = ( pair_name.lower() if isinstance(pair_name, str) - and pair_name.lower() in policy.headers + and ( + pair_name.lower() in policy.headers + or is_credential_name(pair_name, header=True) + ) and isinstance(node.get("value"), str) else None ) @@ -425,7 +472,8 @@ def _redact_node( header_key = lowered if lowered in policy.headers else None if key == "value" and pair_header: header_key = pair_header - if isinstance(item, str) and (header_key or lowered in _SECRET_KEYS): + secret_key = lowered in _SECRET_KEYS or is_credential_name(str(key)) + if isinstance(item, str) and (header_key or secret_key): new = _header_value(header_key or lowered, item) if item else item if item and new != item: reason = f"header:{header_key}" if header_key else f"key:{lowered}" diff --git a/packages/flow/tests/test_report_redaction.py b/packages/flow/tests/test_report_redaction.py index ef483d4b50..afb70e7b57 100644 --- a/packages/flow/tests/test_report_redaction.py +++ b/packages/flow/tests/test_report_redaction.py @@ -378,3 +378,69 @@ def test_hosted_validation_matches_a_redacted_entry_url(): url = _openemr_url(_token()) assert _openemr_url(REDACTED) in _entry_url_forms(url) assert url in _entry_url_forms(url) + + +@pytest.mark.parametrize( + ("app", "text", "name"), + [ + # The motivating case: a per-session CSRF token in a query parameter. + ("OpenEMR", "/interface/main/tabs/main.php?token_main={v}&x=1", "token_main"), + ("OpenEMR", "csrf_token_form={v}&pid=3", "csrf_token_form"), + # The same rule, with no per-app list, on other apps' credential names. + ("Frappe", "X-Frappe-CSRF-Token: {v}", "x-frappe-csrf-token"), + ("EspoCRM", "Espo-Authorization: {v}", "espo-authorization"), + ("any", "/list?next=2&app_sessid={v}", "app_sessid"), + ], +) +def test_credential_names_are_found_by_their_parts_in_any_app(app, text, name): + value = _token() + out, reasons = redact_text(text.format(v=value)) + assert value not in out, app + assert any(reason.endswith(name) for reason in reasons), reasons + + +def test_credential_name_parts_leave_ordinary_names_alone(): + assert not report_redaction.is_credential_name("tokenizer") + assert not report_redaction.is_credential_name("input_tokens") + assert not report_redaction.is_credential_name("patient_id") + text = "/list?tokenizer=bpe&page=2&input_tokens=12" + assert redact_text(text) == (text, []) + + +def test_a_credential_pair_in_a_har_entry_is_redacted(): + value = _token() + report = {"request": {"headers": [{"name": "X-Frappe-CSRF-Token", "value": value}]}} + redacted, found = redact_report(report) + assert value not in json.dumps(redacted) + assert found and found[0].pointer == "/request/headers/0/value" + + +def test_an_approval_named_authorization_is_not_a_credential(): + """Outside a header, 'authorization' names a governed approval.""" + report = {"governed_authorization_id": "approval-123"} + redacted, found = redact_report(report) + assert redacted["governed_authorization_id"] == "approval-123" + assert not found + + +@pytest.mark.parametrize( + "text", + [ + "/app?sessionToken={v}&x=1", + "/app?accessToken={v}", + "/app?__RequestVerificationToken={v}", + "Session-Id: {v}", + "x-session-id: {v}", + ], +) +def test_camel_case_and_two_part_session_names_are_credentials(text): + value = _token() + out, reasons = redact_text(text.format(v=value)) + assert value not in out and reasons + + +def test_report_fields_naming_a_session_stay_readable(): + report = {"dispatch_session_id": "run-42", "authenticated_session_id_sha256": "ab"} + redacted, found = redact_report(report) + assert redacted == report + assert not found