From ef2c164b5cb8d0d60a0cc324fbd3d71cd2570247 Mon Sep 17 00:00:00 2001 From: Anatolii Yatsuk Date: Fri, 28 Aug 2026 19:52:24 +0300 Subject: [PATCH 1/3] fix(oauth): preserve provider error detail on refresh token failure When an OAuth refresh request is rejected, the CDK replaced the provider's own diagnostic with one fixed sentence. `_wrap_refresh_token_exception` already parsed the error body to decide whether the failure was a refresh token failure and then discarded it. For Microsoft Entra that body carries an AADSTS code which separates completely different root causes: AADSTS50173 (grant revoked, e.g. the user changed their password), AADSTS7000218 / AADSTS700025 (client type or secret misconfiguration) and AADSTS50076 / AADSTS50078 / AADSTS700082 (Conditional Access requiring an interactive sign-in). All of them collapsed into the same string, and no AADSTS code reached production failure summaries. The parsed body is now reused instead of being parsed a second time, the full provider response goes to `internal_message` so it lands in the logs, and a short single-line `error` / `error_description` detail is appended after the existing actionable guidance in the user-facing message. Bodies that are empty, non-JSON or not a JSON object degrade to the previous behaviour without raising. Everything surfaced is run through secret redaction, and the authenticator's own refresh token and client secret are redacted explicitly. Co-Authored-By: Claude Opus 5 (1M context) --- .../requests_native_auth/abstract_oauth.py | 135 ++++++++++++-- .../test_requests_native_auth.py | 164 +++++++++++++++++- 2 files changed, 283 insertions(+), 16 deletions(-) diff --git a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py index eae5be13a1..b8a7385dc2 100644 --- a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py +++ b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py @@ -17,7 +17,7 @@ from airbyte_cdk.sources.http_logger import format_http_message from airbyte_cdk.sources.message import MessageRepository, NoopMessageRepository from airbyte_cdk.utils import AirbyteTracedException -from airbyte_cdk.utils.airbyte_secrets_utils import add_to_secrets +from airbyte_cdk.utils.airbyte_secrets_utils import add_to_secrets, filter_secrets from airbyte_cdk.utils.datetime_helpers import AirbyteDateTime, ab_datetime_now, ab_datetime_parse from ..exceptions import DefaultBackoffException @@ -25,6 +25,14 @@ logger = logging.getLogger("airbyte") _NOOP_MESSAGE_REPOSITORY = NoopMessageRepository() +# How much of the provider's error detail is appended to the user-facing message. Long enough to +# keep a provider error code and the start of its description (e.g. Microsoft Entra puts the +# `AADSTS` at the front of `error_description`), short enough to keep the message readable. +_PROVIDER_ERROR_DETAIL_MAX_LENGTH = 200 +# How much of the raw provider response is kept in the internal message, which is logged and is not +# shown to the user. +_PROVIDER_ERROR_RESPONSE_MAX_LENGTH = 1000 + class ResponseKeysMaxRecurtionReached(AirbyteTracedException): """ @@ -238,8 +246,91 @@ def _default_token_expiry_date(self) -> AirbyteDateTime: default_token_expiry_duration_hours = 1 # 1 hour return ab_datetime_now() + timedelta(hours=default_token_expiry_duration_hours) + @staticmethod + def _parse_error_response_content( + response: Optional[requests.Response], + ) -> Optional[Mapping[str, Any]]: + """ + Best-effort parse of an error response body as a JSON object. + + Returns `None` when the response is missing, empty, not valid JSON, or not a JSON object, + so that callers can degrade gracefully instead of raising a new exception while they are + already handling an error. + """ + if response is None: + return None + try: + content = response.json() + except (JSONDecodeError, ValueError): + return None + return content if isinstance(content, Mapping) else None + + def _redact_credentials(self, value: str) -> str: + """ + Redact credential material from a string before it is logged or surfaced to the user. + + Only response bodies are passed here, so request headers (including `Authorization`) are + never echoed. On top of the config secrets already tracked by the CDK, the authenticator's + own refresh token and client secret are redacted explicitly, in case a provider echoes the + submitted credentials back in its error payload. + """ + redacted = filter_secrets(value) + for get_credential in (self.get_refresh_token, self.get_client_secret): + try: + credential = get_credential() + except Exception: + # Never let redaction itself fail the error path we are already in. + continue + if credential and isinstance(credential, str): + redacted = redacted.replace(credential, "****") + return redacted + + @staticmethod + def _truncate(value: str, max_length: int) -> str: + return value if len(value) <= max_length else value[:max_length] + "..." + + def _build_provider_error_detail( + self, response_content: Optional[Mapping[str, Any]] + ) -> Optional[str]: + """ + Build a short, single-line provider error detail suitable for the user-facing message. + + Only the standard OAuth 2.0 error fields (`error` and `error_description`) are used: they + are where providers put the actionable diagnostic, and they are not expected to carry + credentials. For Microsoft Entra this is what distinguishes a revoked grant + (`AADSTS50173`) from a client-type or secret misconfiguration (`AADSTS7000218`, + `AADSTS700025`) from Conditional Access requiring interactive sign-in (`AADSTS50076`, + `AADSTS50078`, `AADSTS700082`) - all of which otherwise collapse into the same message. + """ + if not response_content: + return None + parts = [ + str(response_content[key]) + for key in ("error", "error_description") + if response_content.get(key) + ] + if not parts: + return None + # Collapse newlines and repeated whitespace so the detail stays on a single line. + detail = " ".join(": ".join(parts).split()) + return self._truncate(self._redact_credentials(detail), _PROVIDER_ERROR_DETAIL_MAX_LENGTH) + + def _build_provider_response_info(self, exception: requests.exceptions.RequestException) -> str: + """ + Build the full provider response detail for the internal message, which goes to the logs. + """ + if exception.response is None: + return self._redact_credentials(str(exception)) + body = self._truncate( + self._redact_credentials(exception.response.text), + _PROVIDER_ERROR_RESPONSE_MAX_LENGTH, + ) + return f"HTTP {exception.response.status_code}: {body}" + def _wrap_refresh_token_exception( - self, exception: requests.exceptions.RequestException + self, + exception: requests.exceptions.RequestException, + response_content: Optional[Mapping[str, Any]] = None, ) -> bool: """ Wraps and handles exceptions that occur during the refresh token process. @@ -249,16 +340,20 @@ def _wrap_refresh_token_exception( Args: exception (requests.exceptions.RequestException): The exception raised during the request. + response_content (Optional[Mapping[str, Any]]): The already-parsed response body, when + the caller has one, so the body is not parsed twice. Parsed on demand otherwise. Returns: bool: True if the exception is related to a refresh token error, False otherwise. """ - try: - if exception.response is not None: - exception_content = exception.response.json() - else: - return False - except JSONDecodeError: + if exception.response is None: + return False + exception_content = ( + response_content + if response_content is not None + else self._parse_error_response_content(exception.response) + ) + if exception_content is None: return False return ( exception.response.status_code in self._refresh_token_error_status_codes @@ -333,15 +428,25 @@ def _make_handled_request(self) -> Any: response=e.response, failure_type=FailureType.transient_error, ) - if self._wrap_refresh_token_exception(e): - response_info = ( - f"HTTP {e.response.status_code}: {e.response.text[:1000]}" - if e.response is not None - else str(e) + error_content = self._parse_error_response_content(e.response) + if self._wrap_refresh_token_exception(e, response_content=error_content): + message = ( + "Refresh token was rejected by the OAuth provider (invalid, expired, or " + "already used). Re-authenticate this source's credentials in its connection " + "settings." ) + provider_error_detail = self._build_provider_error_detail(error_content) + if provider_error_detail: + # The provider's own diagnostic is what tells apart otherwise identical-looking + # failures (revoked grant vs. misconfigured client vs. Conditional Access), so + # a short form of it is appended after the actionable guidance. + message = f"{message} Provider error: {provider_error_detail}" raise AirbyteTracedException( - internal_message=f"Refresh token rejected by the OAuth token endpoint. {response_info}", - message="Refresh token was rejected by the OAuth provider (invalid, expired, or already used). Re-authenticate this source's credentials in its connection settings.", + internal_message=( + "Refresh token rejected by the OAuth token endpoint. " + f"{self._build_provider_response_info(e)}" + ), + message=message, failure_type=FailureType.config_error, ) from e raise diff --git a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py index 3a7cb8d3d6..411d81492f 100644 --- a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py +++ b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py @@ -648,9 +648,171 @@ def test_refresh_access_token_wrapped( ) assert f"HTTP {response_code}" in exc_info.value.internal_message assert response_value in exc_info.value.internal_message - assert exc_info.value.message == error_message + assert exc_info.value.message.startswith(error_message) assert exc_info.value.failure_type == FailureType.config_error + def _entra_style_authenticator(self): + return Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + TestOauth2Authenticator.client_secret, + TestOauth2Authenticator.refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="error", + refresh_token_error_values=("invalid_grant", "invalid_client"), + ) + + # Sits at the very end of each provider payload below, past the point where the user-facing + # message is truncated, so it can only be found in the internal message. + trailing_marker = "Correlation ID: 11111111-2222-3333-4444-555555555555" + + @pytest.mark.parametrize( + "error, error_description, expected_code", + ( + ( + # Grant revoked, e.g. the user changed their password: re-authentication is the fix. + "invalid_grant", + "AADSTS50173: The provided grant has expired due to it being revoked, a fresh auth " + "grant is needed. The user might have changed or reset their password.\r\n" + "Trace ID: 00000000-0000-0000-0000-000000000000\r\n" + trailing_marker, + "AADSTS50173", + ), + ( + # Client type / credential misconfiguration: re-authentication will not help. + "invalid_client", + "AADSTS7000218: The request body must contain the following parameter: " + "'client_assertion' or a client credential.\r\n" + trailing_marker, + "AADSTS7000218", + ), + ( + # Conditional Access requires an interactive sign-in. + "invalid_grant", + "AADSTS50076: Due to a configuration change made by your administrator, or because " + "you moved to a new location, you must use multi-factor authentication to access " + "the resource.\r\n" + trailing_marker, + "AADSTS50076", + ), + ), + ) + def test_refresh_access_token_surfaces_provider_error_code( + self, requests_mock, error, error_description, expected_code + ): + """The provider's own diagnostic must reach the logs and the user-facing message.""" + oauth = self._entra_style_authenticator() + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={"error": error, "error_description": error_description}, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + guidance = ( + "Refresh token was rejected by the OAuth provider (invalid, expired, or already used). " + "Re-authenticate this source's credentials in its connection settings." + ) + # The full provider response reaches the internal message, which is logged. + assert expected_code in exc_info.value.internal_message + assert TestOauth2Authenticator.trailing_marker in exc_info.value.internal_message + # The actionable guidance still leads the user-facing message ... + assert exc_info.value.message.startswith(guidance) + # ... followed by a short, single-line provider detail carrying the error code. + assert f"Provider error: {error}: {expected_code}" in exc_info.value.message + assert "\n" not in exc_info.value.message and "\r" not in exc_info.value.message + assert exc_info.value.failure_type == FailureType.config_error + + def test_refresh_access_token_truncates_long_provider_error_detail(self, requests_mock): + oauth = self._entra_style_authenticator() + error_description = "AADSTS50173: " + ("x" * 5000) + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={"error": "invalid_grant", "error_description": error_description}, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + provider_detail = exc_info.value.message.split("Provider error: ", 1)[1] + assert provider_detail.startswith("invalid_grant: AADSTS50173: ") + assert provider_detail.endswith("...") + assert len(provider_detail) < 250 + assert len(exc_info.value.internal_message) < 1200 + + def test_refresh_access_token_redacts_credentials_from_provider_error(self, requests_mock): + """A provider that echoes the submitted credentials must not leak them into the error.""" + oauth = self._entra_style_authenticator() + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={ + "error": "invalid_grant", + "error_description": ( + f"AADSTS50173: rejected refresh_token={TestOauth2Authenticator.refresh_token} " + f"client_secret={TestOauth2Authenticator.client_secret}" + ), + }, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + for message in (exc_info.value.message, exc_info.value.internal_message): + assert TestOauth2Authenticator.refresh_token not in message + assert TestOauth2Authenticator.client_secret not in message + assert "****" in message + assert "AADSTS50173" in exc_info.value.message + + @pytest.mark.parametrize( + "response_kwargs", + ( + {"text": ""}, + {"text": "Bad Request"}, + {"json": ["invalid_grant"]}, + ), + ids=["empty_body", "html_body", "json_array_body"], + ) + def test_refresh_access_token_non_json_body_does_not_raise_new_exception( + self, requests_mock, response_kwargs + ): + """A body we cannot read as a JSON object falls back to the raw RequestException.""" + oauth = self._entra_style_authenticator() + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + **response_kwargs, + ) + + with pytest.raises(RequestException): + oauth.refresh_access_token() + + def test_refresh_access_token_without_error_fields_keeps_bare_guidance(self, requests_mock): + """No usable provider detail means the user-facing message is left exactly as before.""" + oauth = Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + TestOauth2Authenticator.client_secret, + TestOauth2Authenticator.refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="errorCode", + refresh_token_error_values=("invalid_grant",), + ) + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={"errorCode": "invalid_grant", "errorSummary": "the grant was revoked"}, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + assert exc_info.value.message == ( + "Refresh token was rejected by the OAuth provider (invalid, expired, or already used). " + "Re-authenticate this source's credentials in its connection settings." + ) + assert "the grant was revoked" in exc_info.value.internal_message + @freezegun.freeze_time("2022-12-31") class TestSingleUseRefreshTokenOauth2Authenticator: From 6d5e37c9e0b42a78e25b753ba61a7c241379b18b Mon Sep 17 00:00:00 2001 From: "zane.hyatt" Date: Thu, 3 Sep 2026 18:50:07 +0000 Subject: [PATCH 2/3] fix(oauth): keep the user-facing provider error detail deterministic Apply bazarnov's review on #1138, ported from de26718c (#1144): - cap provider detail at 120 chars and keep only the first line of error_description, so Entra trace ids / timestamps stay out of the user-facing message and the same failure yields the same summary - guard error/error_description on isinstance(str) - docstring: AADSTS700082 is an expired/revoked grant; reachability depends on refresh_token_error_* - tests: distinct credential values + update_secrets([]) so the redaction test exercises the explicit pair redaction; assert trailing marker is absent from the user-facing message; add declarative authenticator test Co-authored-by: bazarnov Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../requests_native_auth/abstract_oauth.py | 29 +++++++----- .../sources/declarative/auth/test_oauth.py | 46 ++++++++++++++++++- .../test_requests_native_auth.py | 41 +++++++++++------ 3 files changed, 90 insertions(+), 26 deletions(-) diff --git a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py index b8a7385dc2..ed737684dd 100644 --- a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py +++ b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py @@ -27,8 +27,10 @@ # How much of the provider's error detail is appended to the user-facing message. Long enough to # keep a provider error code and the start of its description (e.g. Microsoft Entra puts the -# `AADSTS` at the front of `error_description`), short enough to keep the message readable. -_PROVIDER_ERROR_DETAIL_MAX_LENGTH = 200 +# `AADSTS` at the front of `error_description`), short enough to keep the message readable +# and to stop before the per-request values (issue dates, trace ids) that some providers embed +# further into the description, so the same failure produces the same message on every attempt. +_PROVIDER_ERROR_DETAIL_MAX_LENGTH = 120 # How much of the raw provider response is kept in the internal message, which is logged and is not # shown to the user. _PROVIDER_ERROR_RESPONSE_MAX_LENGTH = 1000 @@ -295,24 +297,27 @@ def _build_provider_error_detail( """ Build a short, single-line provider error detail suitable for the user-facing message. - Only the standard OAuth 2.0 error fields (`error` and `error_description`) are used: they - are where providers put the actionable diagnostic, and they are not expected to carry - credentials. For Microsoft Entra this is what distinguishes a revoked grant - (`AADSTS50173`) from a client-type or secret misconfiguration (`AADSTS7000218`, - `AADSTS700025`) from Conditional Access requiring interactive sign-in (`AADSTS50076`, - `AADSTS50078`, `AADSTS700082`) - all of which otherwise collapse into the same message. + Only the standard OAuth 2.0 `error` and `error_description` fields (RFC 6749 section 5.2) + are used, and only the first line of the description: providers put the actionable code + there (Microsoft Entra leads with `AADSTS`, which tells a revoked or expired grant + such as `AADSTS50173` / `AADSTS700082` apart from a client misconfiguration such as + `AADSTS7000218`), while per-request trace ids and timestamps follow on later lines. Which + provider errors reach this path at all is set by the authenticator's `refresh_token_error_*` + configuration. """ if not response_content: return None parts = [ - str(response_content[key]) + response_content[key].strip() for key in ("error", "error_description") - if response_content.get(key) + if isinstance(response_content.get(key), str) and response_content[key].strip() ] if not parts: return None - # Collapse newlines and repeated whitespace so the detail stays on a single line. - detail = " ".join(": ".join(parts).split()) + # Keep the first line only and collapse its whitespace: the actionable code leads the + # description, while trace ids and timestamps that differ on every attempt follow on later + # lines and would make the same failure read differently each time. + detail = " ".join(": ".join(parts).splitlines()[0].split()) return self._truncate(self._redact_credentials(detail), _PROVIDER_ERROR_DETAIL_MAX_LENGTH) def _build_provider_response_info(self, exception: requests.exceptions.RequestException) -> str: diff --git a/unit_tests/sources/declarative/auth/test_oauth.py b/unit_tests/sources/declarative/auth/test_oauth.py index 906390c90f..9c3ffe73f2 100644 --- a/unit_tests/sources/declarative/auth/test_oauth.py +++ b/unit_tests/sources/declarative/auth/test_oauth.py @@ -19,7 +19,7 @@ from airbyte_cdk.sources.declarative.auth.jwt import JwtAuthenticator from airbyte_cdk.test.mock_http import HttpMocker, HttpRequest, HttpResponse from airbyte_cdk.utils import AirbyteTracedException -from airbyte_cdk.utils.airbyte_secrets_utils import filter_secrets +from airbyte_cdk.utils.airbyte_secrets_utils import filter_secrets, update_secrets from airbyte_cdk.utils.datetime_helpers import AirbyteDateTime, ab_datetime_now, ab_datetime_parse LOGGER = logging.getLogger(__name__) @@ -687,6 +687,50 @@ def test_error_handling(self, mocker): oauth.refresh_access_token() assert e.value.errno == 400 + def test_refresh_access_token_rejected_surfaces_provider_error_and_redacts(self, requests_mock): + """The declarative authenticator (what manifest connectors use) carries the provider's own + error code into the user-facing message and never the interpolated credentials.""" + update_secrets([]) + oauth = DeclarativeOauth2Authenticator( + token_refresh_endpoint="{{ config['refresh_endpoint'] }}", + client_id="{{ config['client_id'] }}", + client_secret="{{ config['client_secret'] }}", + refresh_token="{{ parameters['refresh_token'] }}", + config=config, + parameters=parameters, + # The defaults the component factory applies when a manifest sets none of these. + refresh_token_error_status_codes=(400,), + refresh_token_error_key="error", + refresh_token_error_values=("invalid_grant", "invalid_permissions"), + ) + requests_mock.post( + config["refresh_endpoint"], + status_code=400, + json={ + "error": "invalid_grant", + "error_description": ( + f"AADSTS50173: Rejected {parameters['refresh_token']} for {config['client_secret']}. " + "The provided grant has expired due to it being revoked, a fresh auth token is " + "needed.\r\nTrace ID: 00000000-0000-0000-0000-000000000000" + ), + }, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + assert exc_info.value.failure_type == FailureType.config_error + assert exc_info.value.message.startswith("Refresh token was rejected by the OAuth provider") + assert ( + "Provider error: invalid_grant: AADSTS50173: Rejected **** for ****." + in exc_info.value.message + ) + assert "Trace ID" not in exc_info.value.message + assert "Trace ID" in exc_info.value.internal_message + for message in (exc_info.value.message, exc_info.value.internal_message): + assert parameters["refresh_token"] not in message + assert config["client_secret"] not in message + def mock_request(method, url, data, headers, **kwargs): if url == "https://refresh_endpoint.com": diff --git a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py index 411d81492f..b03a6180d5 100644 --- a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py +++ b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py @@ -25,9 +25,11 @@ TokenAuthenticator, ) from airbyte_cdk.sources.streams.http.requests_native_auth.abstract_oauth import ( + _PROVIDER_ERROR_DETAIL_MAX_LENGTH, ResponseKeysMaxRecurtionReached, ) from airbyte_cdk.utils import AirbyteTracedException +from airbyte_cdk.utils.airbyte_secrets_utils import update_secrets from airbyte_cdk.utils.datetime_helpers import AirbyteDateTime, ab_datetime_now, ab_datetime_parse LOGGER = logging.getLogger(__name__) @@ -662,8 +664,8 @@ def _entra_style_authenticator(self): refresh_token_error_values=("invalid_grant", "invalid_client"), ) - # Sits at the very end of each provider payload below, past the point where the user-facing - # message is truncated, so it can only be found in the internal message. + # Sits on a later line of each provider payload below, like the trace ids and timestamps + # Microsoft Entra appends, so it must reach the internal message and never the user-facing one. trailing_marker = "Correlation ID: 11111111-2222-3333-4444-555555555555" @pytest.mark.parametrize( @@ -673,7 +675,7 @@ def _entra_style_authenticator(self): # Grant revoked, e.g. the user changed their password: re-authentication is the fix. "invalid_grant", "AADSTS50173: The provided grant has expired due to it being revoked, a fresh auth " - "grant is needed. The user might have changed or reset their password.\r\n" + "token is needed. The user might have changed or reset their password.\r\n" "Trace ID: 00000000-0000-0000-0000-000000000000\r\n" + trailing_marker, "AADSTS50173", ), @@ -681,7 +683,7 @@ def _entra_style_authenticator(self): # Client type / credential misconfiguration: re-authentication will not help. "invalid_client", "AADSTS7000218: The request body must contain the following parameter: " - "'client_assertion' or a client credential.\r\n" + trailing_marker, + "'client_assertion' or 'client_secret'.\r\n" + trailing_marker, "AADSTS7000218", ), ( @@ -720,6 +722,9 @@ def test_refresh_access_token_surfaces_provider_error_code( # ... followed by a short, single-line provider detail carrying the error code. assert f"Provider error: {error}: {expected_code}" in exc_info.value.message assert "\n" not in exc_info.value.message and "\r" not in exc_info.value.message + # Later lines of the description (trace ids, timestamps) stay out of the user-facing message, + # so the same failure produces the same message on every attempt. + assert TestOauth2Authenticator.trailing_marker not in exc_info.value.message assert exc_info.value.failure_type == FailureType.config_error def test_refresh_access_token_truncates_long_provider_error_detail(self, requests_mock): @@ -737,21 +742,31 @@ def test_refresh_access_token_truncates_long_provider_error_detail(self, request provider_detail = exc_info.value.message.split("Provider error: ", 1)[1] assert provider_detail.startswith("invalid_grant: AADSTS50173: ") assert provider_detail.endswith("...") - assert len(provider_detail) < 250 + assert len(provider_detail) == _PROVIDER_ERROR_DETAIL_MAX_LENGTH + len("...") assert len(exc_info.value.internal_message) < 1200 def test_refresh_access_token_redacts_credentials_from_provider_error(self, requests_mock): """A provider that echoes the submitted credentials must not leak them into the error.""" - oauth = self._entra_style_authenticator() + # Start from no config-level secrets: earlier tests register values such as "token" through + # add_to_secrets, which would otherwise mask the credentials on their own. + update_secrets([]) + refresh_token = "0.AXoA-rt-9f3ZqW7kP2" + client_secret = "s3cr3t~Xyz-1Qp" + oauth = Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + client_secret, + refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="error", + refresh_token_error_values=("invalid_grant",), + ) requests_mock.post( f"https://{TestOauth2Authenticator.refresh_endpoint}", status_code=400, json={ "error": "invalid_grant", - "error_description": ( - f"AADSTS50173: rejected refresh_token={TestOauth2Authenticator.refresh_token} " - f"client_secret={TestOauth2Authenticator.client_secret}" - ), + "error_description": f"AADSTS50173: rejected rt={refresh_token} cs={client_secret}", }, ) @@ -759,9 +774,9 @@ def test_refresh_access_token_redacts_credentials_from_provider_error(self, requ oauth.refresh_access_token() for message in (exc_info.value.message, exc_info.value.internal_message): - assert TestOauth2Authenticator.refresh_token not in message - assert TestOauth2Authenticator.client_secret not in message - assert "****" in message + assert refresh_token not in message + assert client_secret not in message + assert message.count("****") == 2 assert "AADSTS50173" in exc_info.value.message @pytest.mark.parametrize( From 1216c9d2a9ce322d52799e722790afcb01721ef0 Mon Sep 17 00:00:00 2001 From: "zane.hyatt" Date: Wed, 9 Sep 2026 04:14:29 +0000 Subject: [PATCH 3/3] fix(oauth): append only the provider error code, not the description prose AADSTS700082 embeds the token issue timestamp in the first sentence of error_description, so neither the first-line rule nor the 120-char cap could remove it: two attempts at the same failure produced two different user-facing messages, moving failure-summary cardinality for that code from exactly 1 to unbounded. Emit the RFC 6749 error token plus only the leading provider code instead, which is deterministic by construction. Also closes the coverage gap: the two previous guards masked each other and neither failed on its own. Six mutants now each fail individually. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../requests_native_auth/abstract_oauth.py | 58 +++--- .../sources/declarative/auth/test_oauth.py | 11 +- .../test_requests_native_auth.py | 180 +++++++++++++++++- 3 files changed, 216 insertions(+), 33 deletions(-) diff --git a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py index ed737684dd..e236463814 100644 --- a/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py +++ b/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py @@ -3,6 +3,7 @@ # import logging +import re import threading from abc import abstractmethod from datetime import timedelta @@ -25,12 +26,15 @@ logger = logging.getLogger("airbyte") _NOOP_MESSAGE_REPOSITORY = NoopMessageRepository() -# How much of the provider's error detail is appended to the user-facing message. Long enough to -# keep a provider error code and the start of its description (e.g. Microsoft Entra puts the -# `AADSTS` at the front of `error_description`), short enough to keep the message readable -# and to stop before the per-request values (issue dates, trace ids) that some providers embed -# further into the description, so the same failure produces the same message on every attempt. -_PROVIDER_ERROR_DETAIL_MAX_LENGTH = 120 +# Provider error codes that lead `error_description`, e.g. Microsoft Entra's `AADSTS50173`. +# Only the code is appended to the user-facing message: it is the entire grouping key, and unlike +# the surrounding prose it is identical on every attempt at the same failure. +_PROVIDER_ERROR_CODE_PATTERN = re.compile(r"^[A-Za-z]{3,}\d{4,}\b") +# Upper bound on the provider-controlled text appended to the user-facing message. Both the +# RFC 6749 `error` token and the extracted code come from the provider, so both are still capped. +# The longest standard token (`unsupported_grant_type`, 22) plus a 7-digit Entra code is 37 +# characters, so this leaves roughly 2x headroom while still bounding a misbehaving provider. +_PROVIDER_ERROR_DETAIL_MAX_LENGTH = 64 # How much of the raw provider response is kept in the internal message, which is logged and is not # shown to the user. _PROVIDER_ERROR_RESPONSE_MAX_LENGTH = 1000 @@ -295,30 +299,36 @@ def _build_provider_error_detail( self, response_content: Optional[Mapping[str, Any]] ) -> Optional[str]: """ - Build a short, single-line provider error detail suitable for the user-facing message. + Build a short, deterministic provider error detail for the user-facing message. - Only the standard OAuth 2.0 `error` and `error_description` fields (RFC 6749 section 5.2) - are used, and only the first line of the description: providers put the actionable code - there (Microsoft Entra leads with `AADSTS`, which tells a revoked or expired grant - such as `AADSTS50173` / `AADSTS700082` apart from a client misconfiguration such as - `AADSTS7000218`), while per-request trace ids and timestamps follow on later lines. Which - provider errors reach this path at all is set by the authenticator's `refresh_token_error_*` - configuration. + Only the standard OAuth 2.0 `error` field (RFC 6749 section 5.2) and the provider error + code leading `error_description` are used. Both are stable for a given failure, so the same + failure produces a byte-identical message on every attempt and the platform groups them + into a single failure summary. The description prose is deliberately excluded: it is + free-form, and providers embed per-request values in it -- Microsoft Entra's + `AADSTS700082` carries the token issue timestamp in its first sentence -- which would make + the grouping key unbounded. The code alone is what distinguishes a revoked grant + (`AADSTS50173`) from a misconfigured client (`AADSTS7000218`) or a Conditional Access + requirement (`AADSTS50076`). The full response body is preserved in the internal message, + which is logged. Which provider errors reach this path at all is set by the + authenticator's `refresh_token_error_*` configuration. """ if not response_content: return None - parts = [ - response_content[key].strip() - for key in ("error", "error_description") - if isinstance(response_content.get(key), str) and response_content[key].strip() - ] + parts = [] + error = response_content.get("error") + if isinstance(error, str) and error.strip(): + parts.append(" ".join(error.split())) + description = response_content.get("error_description") + if isinstance(description, str): + code_match = _PROVIDER_ERROR_CODE_PATTERN.match(description.strip()) + if code_match: + parts.append(code_match.group()) if not parts: return None - # Keep the first line only and collapse its whitespace: the actionable code leads the - # description, while trace ids and timestamps that differ on every attempt follow on later - # lines and would make the same failure read differently each time. - detail = " ".join(": ".join(parts).splitlines()[0].split()) - return self._truncate(self._redact_credentials(detail), _PROVIDER_ERROR_DETAIL_MAX_LENGTH) + return self._truncate( + self._redact_credentials(": ".join(parts)), _PROVIDER_ERROR_DETAIL_MAX_LENGTH + ) def _build_provider_response_info(self, exception: requests.exceptions.RequestException) -> str: """ diff --git a/unit_tests/sources/declarative/auth/test_oauth.py b/unit_tests/sources/declarative/auth/test_oauth.py index 9c3ffe73f2..ddd96a5474 100644 --- a/unit_tests/sources/declarative/auth/test_oauth.py +++ b/unit_tests/sources/declarative/auth/test_oauth.py @@ -721,11 +721,14 @@ def test_refresh_access_token_rejected_surfaces_provider_error_and_redacts(self, assert exc_info.value.failure_type == FailureType.config_error assert exc_info.value.message.startswith("Refresh token was rejected by the OAuth provider") - assert ( - "Provider error: invalid_grant: AADSTS50173: Rejected **** for ****." - in exc_info.value.message - ) + # Only the provider code is surfaced, so the echoed credentials cannot reach the + # user-facing message at all -- they are absent by construction, not merely masked. + assert exc_info.value.message.endswith("Provider error: invalid_grant: AADSTS50173") + assert "****" not in exc_info.value.message + assert "Rejected" not in exc_info.value.message assert "Trace ID" not in exc_info.value.message + # The internal message keeps the full body with both credentials redacted. + assert exc_info.value.internal_message.count("****") == 2 assert "Trace ID" in exc_info.value.internal_message for message in (exc_info.value.message, exc_info.value.internal_message): assert parameters["refresh_token"] not in message diff --git a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py index b03a6180d5..98450e7f01 100644 --- a/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py +++ b/unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py @@ -4,6 +4,7 @@ import json import logging +import re import threading import time from datetime import timedelta @@ -727,7 +728,8 @@ def test_refresh_access_token_surfaces_provider_error_code( assert TestOauth2Authenticator.trailing_marker not in exc_info.value.message assert exc_info.value.failure_type == FailureType.config_error - def test_refresh_access_token_truncates_long_provider_error_detail(self, requests_mock): + def test_refresh_access_token_omits_description_prose(self, requests_mock): + """Only the provider code is surfaced; the surrounding prose never reaches the user.""" oauth = self._entra_style_authenticator() error_description = "AADSTS50173: " + ("x" * 5000) requests_mock.post( @@ -736,14 +738,50 @@ def test_refresh_access_token_truncates_long_provider_error_detail(self, request json={"error": "invalid_grant", "error_description": error_description}, ) + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + assert exc_info.value.message.endswith("Provider error: invalid_grant: AADSTS50173") + assert "xxx" not in exc_info.value.message + assert len(exc_info.value.internal_message) < 1200 + + def test_provider_error_detail_is_capped(self, requests_mock): + """The `error` field is not always the gated one, so it stays provider-controlled. + + `_wrap_refresh_token_exception` gates on `refresh_token_error_key`, but the user-facing + detail always reads the RFC 6749 `error` field. A connector configured against a different + key (Okta's `errorCode`, here) therefore admits an arbitrary `error` value, which is what + the cap bounds. + + The expected length is written as a literal rather than derived from the imported + constant, so raising the cap fails this test instead of silently redefining it. + """ + oauth = Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + TestOauth2Authenticator.client_secret, + TestOauth2Authenticator.refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="errorCode", + refresh_token_error_values=("invalid_grant",), + ) + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={ + "errorCode": "invalid_grant", + "error": "e" * 500, + "error_description": "AADSTS50173: y", + }, + ) + with pytest.raises(AirbyteTracedException) as exc_info: oauth.refresh_access_token() provider_detail = exc_info.value.message.split("Provider error: ", 1)[1] - assert provider_detail.startswith("invalid_grant: AADSTS50173: ") assert provider_detail.endswith("...") + assert len(provider_detail) == 67 # 64-char cap + the "..." marker assert len(provider_detail) == _PROVIDER_ERROR_DETAIL_MAX_LENGTH + len("...") - assert len(exc_info.value.internal_message) < 1200 def test_refresh_access_token_redacts_credentials_from_provider_error(self, requests_mock): """A provider that echoes the submitted credentials must not leak them into the error.""" @@ -776,8 +814,12 @@ def test_refresh_access_token_redacts_credentials_from_provider_error(self, requ for message in (exc_info.value.message, exc_info.value.internal_message): assert refresh_token not in message assert client_secret not in message - assert message.count("****") == 2 - assert "AADSTS50173" in exc_info.value.message + # The internal message keeps the echoed body, with both credentials redacted. + assert exc_info.value.internal_message.count("****") == 2 + # The user-facing message never carries description prose, so there is nothing to redact: + # credential echo-back is structurally impossible there, not merely masked. + assert "****" not in exc_info.value.message + assert exc_info.value.message.endswith("Provider error: invalid_grant: AADSTS50173") @pytest.mark.parametrize( "response_kwargs", @@ -802,6 +844,134 @@ def test_refresh_access_token_non_json_body_does_not_raise_new_exception( with pytest.raises(RequestException): oauth.refresh_access_token() + def _message_for_description(self, requests_mock, error_description): + oauth = self._entra_style_authenticator() + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={"error": "invalid_grant", "error_description": error_description}, + ) + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + return exc_info.value.message + + # AADSTS700082 embeds the token issue timestamp in the *first sentence* of the description, + # so no first-line rule and no character cap can separate it from the stable text. + _AADSTS700082 = ( + "AADSTS700082: The refresh token has expired due to inactivity. The token was issued " + "on {issued} and was inactive for 90.00:00:00." + ) + + def test_same_failure_produces_identical_message_across_attempts(self, requests_mock): + """The grouping property: per-request values must not vary the user-facing message.""" + first = self._message_for_description( + requests_mock, self._AADSTS700082.format(issued="2026-06-11T03:14:07.000Z") + ) + second = self._message_for_description( + requests_mock, self._AADSTS700082.format(issued="2026-07-29T22:01:52.000Z") + ) + assert first == second + assert "AADSTS700082" in first + + def test_timestamp_in_first_sentence_never_reaches_message(self, requests_mock): + """A per-request value inside the first sentence, with no later lines to fall back on.""" + message = self._message_for_description( + requests_mock, self._AADSTS700082.format(issued="2026-06-11T03:14:07.000Z") + ) + assert "AADSTS700082" in message + assert re.search(r"\d{4}-\d{2}-\d{2}T", message) is None + + def test_trace_id_under_the_cap_never_reaches_message(self, requests_mock): + """A short payload whose trace id sits below the cap, so the cap alone cannot remove it.""" + message = self._message_for_description( + requests_mock, + "AADSTS50173: grant revoked.\r\nTrace ID: 00000000-0000-0000-0000-000000000000", + ) + assert "AADSTS50173" in message + assert "Trace ID" not in message + + def test_provider_code_is_only_read_from_the_start_of_the_description(self, requests_mock): + """The code pattern is anchored: a code mentioned mid-prose is not the failure's own code. + + Kills the `match` -> `search` mutant. Matching anywhere would let a code named inside a + sentence (a doc reference, a nested provider error) become the grouping key. + """ + message = self._message_for_description( + requests_mock, "Something went wrong; see AADSTS50173 in the documentation." + ) + assert message.endswith("Provider error: invalid_grant") + assert "AADSTS50173" not in message + + def test_credential_extracted_as_a_provider_code_is_redacted(self, requests_mock): + """A code-shaped refresh token leading the description is extracted, so it must be masked.""" + update_secrets([]) + refresh_token = "RTOKEN1234567890" + oauth = Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + TestOauth2Authenticator.client_secret, + refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="error", + refresh_token_error_values=("invalid_grant",), + ) + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={"error": "invalid_grant", "error_description": f"{refresh_token} was rejected."}, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + assert refresh_token not in exc_info.value.message + assert exc_info.value.message.endswith("Provider error: invalid_grant: ****") + + def test_provider_code_requires_a_word_boundary(self, requests_mock): + """`AADSTS50173abc` is a longer identifier, not the code `AADSTS50173`.""" + message = self._message_for_description(requests_mock, "AADSTS50173abc: not a code.") + assert message.endswith("Provider error: invalid_grant") + assert "AADSTS50173" not in message + + def test_credentials_echoed_in_the_error_field_are_redacted(self, requests_mock): + """The `error` field is ungated when the connector keys off a different field. + + Kills the mutant that drops `_redact_credentials` from the user-facing detail: the code and + the gated error token are safe by construction, but this field is not. + """ + update_secrets([]) + refresh_token = "0.AXoA-rt-9f3ZqW7kP2" + oauth = Oauth2Authenticator( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + TestOauth2Authenticator.client_id, + TestOauth2Authenticator.client_secret, + refresh_token, + refresh_token_error_status_codes=(400,), + refresh_token_error_key="errorCode", + refresh_token_error_values=("invalid_grant",), + ) + requests_mock.post( + f"https://{TestOauth2Authenticator.refresh_endpoint}", + status_code=400, + json={ + "errorCode": "invalid_grant", + "error": f"rejected {refresh_token}", + "error_description": "AADSTS50173: revoked.", + }, + ) + + with pytest.raises(AirbyteTracedException) as exc_info: + oauth.refresh_access_token() + + assert refresh_token not in exc_info.value.message + assert "****" in exc_info.value.message + + def test_description_without_provider_code_falls_back_to_error_token(self, requests_mock): + """Providers that return prose with no code still get the RFC 6749 error token.""" + message = self._message_for_description(requests_mock, "Token has been expired or revoked.") + assert message.endswith("Provider error: invalid_grant") + assert "revoked." not in message + def test_refresh_access_token_without_error_fields_keeps_bare_guidance(self, requests_mock): """No usable provider detail means the user-facing message is left exactly as before.""" oauth = Oauth2Authenticator(