-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Python: Fix: Parse oauth_consent_request events in Azure AI client #4197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -588,6 +588,61 @@ def _get_current_conversation_id(self, options: Mapping[str, Any], **kwargs: Any | |||||||||||
| """Get the current conversation ID from chat options or kwargs.""" | ||||||||||||
| return options.get("conversation_id") or kwargs.get("conversation_id") or self.conversation_id | ||||||||||||
|
|
||||||||||||
| @override | ||||||||||||
| def _parse_response_from_openai( | ||||||||||||
| self, | ||||||||||||
| response: Any, | ||||||||||||
| options: dict[str, Any], | ||||||||||||
| ) -> ChatResponse: | ||||||||||||
| """Parse an Azure AI Responses API response, handling Azure-specific output item types.""" | ||||||||||||
| result = super()._parse_response_from_openai(response, options) | ||||||||||||
|
|
||||||||||||
| for item in response.output: | ||||||||||||
| if item.type == "oauth_consent_request": | ||||||||||||
| consent_link = item.consent_link | ||||||||||||
| if consent_link: | ||||||||||||
| result.messages[0].contents.append( | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. result.messages[0] is accessed unconditionally. If the parent _parse_response_from_openai returns a ChatResponse with an empty messages list (e.g. on an unexpected response shape), this raises IndexError.
Suggested change
|
||||||||||||
| Content.from_oauth_consent_request( | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| consent_link=consent_link, | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consent_link is passed to the frontend without URL validation. If the Azure API returns a javascript: URI or a non-HTTPS URL, the frontend will receive it verbatim and could render it as a clickable link (XSS / phishing). Validate the scheme before using the value.
Suggested change
|
||||||||||||
| raw_representation=item, | ||||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
| elif not consent_link: | ||||||||||||
| logger.warning("Received oauth_consent_request output without consent_link: %s", item) | ||||||||||||
|
|
||||||||||||
| return result | ||||||||||||
|
|
||||||||||||
| @override | ||||||||||||
| def _parse_chunk_from_openai( | ||||||||||||
| self, | ||||||||||||
| event: Any, | ||||||||||||
| options: dict[str, Any], | ||||||||||||
| function_call_ids: dict[int, tuple[str, str]], | ||||||||||||
| ) -> ChatResponseUpdate: | ||||||||||||
| """Parse an Azure AI streaming event, handling Azure-specific event types.""" | ||||||||||||
| # Intercept output_item.added events for Azure-specific item types | ||||||||||||
| if event.type == "response.output_item.added" and event.item.type == "oauth_consent_request": | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and this logic will silently ignore other content types? And that seems inconsistent with the non-streaming flow?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When |
||||||||||||
| event_item = event.item | ||||||||||||
| consent_link = event_item.consent_link | ||||||||||||
| contents: list[Content] = [] | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same missing HTTPS scheme validation as the non-streaming path — consent_link should be checked for a safe URL scheme before emitting.
Suggested change
|
||||||||||||
| if consent_link: | ||||||||||||
| contents.append( | ||||||||||||
| Content.from_oauth_consent_request( | ||||||||||||
| consent_link=consent_link, | ||||||||||||
| raw_representation=event_item, | ||||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
| else: | ||||||||||||
| logger.warning("Received oauth_consent_request output without consent_link: %s", event_item) | ||||||||||||
| return ChatResponseUpdate( | ||||||||||||
| contents=contents, | ||||||||||||
| role="assistant", | ||||||||||||
| model_id=self.model_id, | ||||||||||||
| raw_representation=event, | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| return super()._parse_chunk_from_openai(event, options, function_call_ids) | ||||||||||||
|
|
||||||||||||
| def _prepare_messages_for_azure_ai(self, messages: Sequence[Message]) -> tuple[list[Message], str | None]: | ||||||||||||
| """Prepare input from messages and convert system/developer messages to instructions.""" | ||||||||||||
| result: list[Message] = [] | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2146,3 +2146,63 @@ def test_build_url_citation_content_with_dict(mock_project_client: MagicMock) -> | |
|
|
||
|
|
||
| # endregion | ||
|
|
||
|
|
||
| # region OAuth Consent | ||
|
|
||
|
|
||
| def test_parse_chunk_with_oauth_consent_request(mock_project_client: MagicMock) -> None: | ||
| """Test that a streaming oauth_consent_request output item is parsed into oauth_consent_request content. | ||
|
|
||
| This reproduces the bug from issue #3950 where the event was logged as "Unparsed event" | ||
| and silently discarded, causing the agent run to complete with zero content. | ||
| """ | ||
| client = AzureAIClient(project_client=mock_project_client, agent_name="test") | ||
| chat_options: dict[str, Any] = {} | ||
| function_call_ids: dict[int, tuple[str, str]] = {} | ||
|
|
||
| mock_item = MagicMock() | ||
| mock_item.type = "oauth_consent_request" | ||
| mock_item.consent_link = "https://login.microsoftonline.com/common/oauth2/authorize?client_id=abc123" | ||
|
|
||
| mock_event = MagicMock() | ||
| mock_event.type = "response.output_item.added" | ||
| mock_event.item = mock_item | ||
| mock_event.output_index = 0 | ||
|
|
||
| update = client._parse_chunk_from_openai(mock_event, chat_options, function_call_ids) | ||
|
|
||
| assert len(update.contents) == 1 | ||
| consent_content = update.contents[0] | ||
| assert consent_content.type == "oauth_consent_request" | ||
| assert consent_content.consent_link == "https://login.microsoftonline.com/common/oauth2/authorize?client_id=abc123" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The no-consent-link branch of |
||
| assert consent_content.user_input_request is True | ||
|
|
||
|
|
||
| def test_parse_response_with_oauth_consent_output_item(mock_project_client: MagicMock) -> None: | ||
| """Test that a non-streaming oauth_consent_request output item is parsed correctly.""" | ||
| client = AzureAIClient(project_client=mock_project_client, agent_name="test") | ||
|
|
||
| mock_item = MagicMock() | ||
| mock_item.type = "oauth_consent_request" | ||
| mock_item.consent_link = "https://login.microsoftonline.com/consent?code=abc" | ||
|
|
||
| mock_response = MagicMock() | ||
| mock_response.output = [mock_item] | ||
| mock_response.output_parsed = None | ||
| mock_response.metadata = {} | ||
| mock_response.id = "resp-oauth-1" | ||
| mock_response.model = "test-model" | ||
| mock_response.created_at = 1000000000 | ||
| mock_response.usage = None | ||
| mock_response.status = "completed" | ||
|
|
||
| response = client._parse_response_from_openai(mock_response, {}) | ||
|
|
||
| assert len(response.messages) > 0 | ||
| consent_contents = [c for c in response.messages[0].contents if c.type == "oauth_consent_request"] | ||
| assert len(consent_contents) == 1 | ||
| assert consent_contents[0].consent_link == "https://login.microsoftonline.com/consent?code=abc" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The no-consent-link branch of |
||
|
|
||
|
|
||
| # endregion | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test constructs Content directly with
user_input_request=Truebut noconsent_link, which works but is inconsistent with how real no-link content would arrive (the factory always sets consent_link). Consider usingContent('oauth_consent_request')(no extra args) to better represent a malformed/missing-link scenario, or document whyuser_input_request=Trueis set here.