Repository navigation
[PR-verification] fix(oauth): deterministic provider error detail on refresh-token rejection - #1144
Closed
Baz (bazarnov) wants to merge 3 commits into
Closed
Baz (bazarnov) wants to merge 3 commits into
Baz (bazarnov) wants to merge 3 commits into
Conversation
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) <noreply@anthropic.com>
- Append only the first line of `error_description`, capped at 120 characters: Microsoft Entra puts the AADSTS code and its sentence there and follows with per-request Trace ID, Correlation ID, Timestamp and issue dates, which otherwise made the same failure produce a different message on every attempt and defeated grouping by failure summary. The full body still lands in `internal_message`. - Skip non-string `error` / `error_description` values instead of rendering their repr. - Docstring: AADSTS700082 is refresh-token expiry due to inactivity, not Conditional Access (https://learn.microsoft.com/en-us/entra/identity-platform/reference-error-codes); which provider errors reach this path at all is set by `refresh_token_error_*`. - Tests: assert the trailing lines never reach `message`; the redaction test now uses distinct credential values and resets config-level secrets first (the previous values equalled their field names and a secret leaked from an earlier test masked them on its own); add the declarative-authenticator path used by manifest connectors; align Entra fixtures with the official wording.
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@verify/oauth-provider-error-detail-first-line#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch verify/oauth-provider-error-detail-first-linePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Full)4 376 tests 4 364 ✅ 13m 34s ⏱️ Results for commit de26718. |
Baz (bazarnov)
deleted the
verify/oauth-provider-error-detail-first-line
branch
September 3, 2026 16:32
devin-ai-integration Bot
added a commit
that referenced
this pull request
Sep 3, 2026
Apply bazarnov's review on #1138, ported from de26718 (#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 <bazarnov@users.noreply.github.com> Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Validation run for a CDK OAuth error-detail change: a deterministic single-line provider detail is appended to the refresh-token rejection message, with the full provider body kept in
internal_message. Not for merge.