Skip to content

Commit 1cda09d

Browse files
CopilotsergioescaleranikhilNava
authored
Fix domain override validation to accept valid URLs with protocols (#112)
* Initial plan * Fix domain override validation to accept valid URLs - Updated get_validated_domain_override() to accept both domain-only and full URL formats - Added URL parsing logic to validate http/https protocols - Updated URL construction in agent365_exporter.py to handle both formats - Added comprehensive unit tests for domain validation - Updated existing tests to reflect new validation behavior Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com> * Fix URL construction for domain with port - Updated URL construction logic to check for '://' to distinguish between real protocols and domain:port format - Added test for domain with port (example.com:8080) to ensure https:// is prepended correctly - All 28 tests pass Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com> * Add clarifying comments for urlparse edge case - Added comments explaining why checking for '://' is necessary - urlparse treats 'example.com:8080' as having scheme='example.com', so we need to check for '://' to distinguish between real protocols and domain:port format - All 28 tests still passing Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com> * Fix lint errors and address code review feedback - Remove trailing whitespace (auto-formatted with ruff) - Add validation for malformed URLs like "http:8080" (missing slashes) - Update comment to clarify "domain with optional port" instead of "just a domain" - Add tests for malformed URL edge cases - All 30 tests passing Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sergioescalera <8428450+sergioescalera@users.noreply.github.com> Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com>
1 parent f47b62b commit 1cda09d

4 files changed

Lines changed: 247 additions & 11 deletions

File tree

‎libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/agent365_exporter.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import time
1111
from collections.abc import Callable, Sequence
1212
from typing import Any, final
13+
from urllib.parse import urlparse
1314

1415
import requests
1516
from microsoft_agents_a365.runtime.power_platform_api_discovery import PowerPlatformApiDiscovery
@@ -94,12 +95,24 @@ def export(self, spans: Sequence[ReadableSpan]) -> SpanExportResult:
9495
else:
9596
discovery = PowerPlatformApiDiscovery(self._cluster_category)
9697
endpoint = discovery.get_tenant_island_cluster_endpoint(tenant_id)
98+
9799
endpoint_path = (
98100
f"/maven/agent365/service/agents/{agent_id}/traces"
99101
if self._use_s2s_endpoint
100102
else f"/maven/agent365/agents/{agent_id}/traces"
101103
)
102-
url = f"https://{endpoint}{endpoint_path}?api-version=1"
104+
105+
# Construct URL - if endpoint has a scheme (http:// or https://), use it as-is
106+
# Otherwise, prepend https://
107+
# Note: Check for "://" to distinguish between real protocols and domain:port format
108+
# (urlparse treats "example.com:8080" as having scheme="example.com")
109+
parsed = urlparse(endpoint)
110+
if parsed.scheme and "://" in endpoint:
111+
# Endpoint is a full URL, append path
112+
url = f"{endpoint}{endpoint_path}?api-version=1"
113+
else:
114+
# Endpoint is just a domain (possibly with port), prepend https://
115+
url = f"https://{endpoint}{endpoint_path}?api-version=1"
103116

104117
# Debug: Log endpoint being used
105118
logger.info(

‎libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/utils.py‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import os
66
from collections.abc import Sequence
77
from typing import Any
8+
from urllib.parse import urlparse
89

910
from opentelemetry.sdk.trace import ReadableSpan
1011
from opentelemetry.trace import SpanKind, StatusCode
@@ -153,12 +154,43 @@ def get_validated_domain_override() -> str | None:
153154
if not domain_override:
154155
return None
155156

156-
# Basic validation: ensure domain doesn't contain protocol or path separators
157-
if "://" in domain_override or "/" in domain_override:
158-
logger.warning(
159-
f"Invalid domain override '{domain_override}': "
160-
"domain should not contain protocol (://) or path separators (/)"
161-
)
157+
# Validate that it's a valid URL
158+
try:
159+
parsed = urlparse(domain_override)
160+
161+
# If scheme is present and looks like a protocol (contains //)
162+
# Note: We check for "://" because urlparse treats "example.com:8080" as having
163+
# scheme="example.com", but this is actually a domain with port, not a protocol.
164+
if parsed.scheme and "://" in domain_override:
165+
# Validate it's http or https
166+
if parsed.scheme not in ("http", "https"):
167+
logger.warning(
168+
f"Invalid domain override '{domain_override}': "
169+
f"scheme must be http or https, got '{parsed.scheme}'"
170+
)
171+
return None
172+
# Must have a netloc (hostname) when scheme is present
173+
if not parsed.netloc:
174+
logger.warning(f"Invalid domain override '{domain_override}': missing hostname")
175+
return None
176+
else:
177+
# If no scheme with ://, it should be a domain with optional port (no path)
178+
# Note: domain can contain : for port (e.g., example.com:8080)
179+
# Reject malformed URLs like "http:8080" that look like protocols but aren't
180+
if domain_override.startswith(("http:", "https:")) and "://" not in domain_override:
181+
logger.warning(
182+
f"Invalid domain override '{domain_override}': "
183+
"malformed URL - protocol requires '://'"
184+
)
185+
return None
186+
if "/" in domain_override:
187+
logger.warning(
188+
f"Invalid domain override '{domain_override}': "
189+
"domain without protocol should not contain path separators (/)"
190+
)
191+
return None
192+
except Exception as e:
193+
logger.warning(f"Invalid domain override '{domain_override}': {e}")
162194
return None
163195

164196
return domain_override

‎tests/observability/core/exporters/test_utils.py‎

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
# Copyright (c) Microsoft Corporation.
2-
# Licensed under the MIT License.
1+
# Copyright (c) Microsoft. All rights reserved.
32

3+
import os
44
import unittest
5+
from unittest.mock import patch
56

67
from microsoft_agents_a365.observability.core.exporters.utils import (
8+
get_validated_domain_override,
79
truncate_span,
810
)
911

@@ -64,5 +66,89 @@ def test_truncate_span_if_needed(self):
6466
self.assertEqual(result["attributes"][key], "TRUNCATED")
6567

6668

69+
class TestGetValidatedDomainOverride(unittest.TestCase):
70+
"""Unit tests for get_validated_domain_override function."""
71+
72+
def test_returns_none_when_env_var_not_set(self):
73+
"""Test that function returns None when environment variable is not set."""
74+
with patch.dict(os.environ, {}, clear=True):
75+
result = get_validated_domain_override()
76+
self.assertIsNone(result)
77+
78+
def test_returns_none_when_env_var_is_empty(self):
79+
"""Test that function returns None when environment variable is empty."""
80+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": ""}):
81+
result = get_validated_domain_override()
82+
self.assertIsNone(result)
83+
84+
def test_returns_none_when_env_var_is_whitespace(self):
85+
"""Test that function returns None when environment variable is only whitespace."""
86+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": " "}):
87+
result = get_validated_domain_override()
88+
self.assertIsNone(result)
89+
90+
def test_accepts_valid_domain(self):
91+
"""Test that function accepts a valid domain without protocol."""
92+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "example.com"}):
93+
result = get_validated_domain_override()
94+
self.assertEqual(result, "example.com")
95+
96+
def test_accepts_valid_domain_with_port(self):
97+
"""Test that function accepts a valid domain with port."""
98+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "example.com:8080"}):
99+
result = get_validated_domain_override()
100+
self.assertEqual(result, "example.com:8080")
101+
102+
def test_accepts_valid_https_url(self):
103+
"""Test that function accepts a valid URL with https protocol."""
104+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "https://example.com"}):
105+
result = get_validated_domain_override()
106+
self.assertEqual(result, "https://example.com")
107+
108+
def test_accepts_valid_http_url(self):
109+
"""Test that function accepts a valid URL with http protocol."""
110+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "http://example.com"}):
111+
result = get_validated_domain_override()
112+
self.assertEqual(result, "http://example.com")
113+
114+
def test_accepts_valid_http_url_with_port(self):
115+
"""Test that function accepts a valid URL with http protocol and port."""
116+
with patch.dict(
117+
os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "http://localhost:8080"}
118+
):
119+
result = get_validated_domain_override()
120+
self.assertEqual(result, "http://localhost:8080")
121+
122+
def test_rejects_invalid_protocol(self):
123+
"""Test that function rejects URLs with invalid protocols (not http/https)."""
124+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "ftp://example.com"}):
125+
result = get_validated_domain_override()
126+
self.assertIsNone(result)
127+
128+
def test_rejects_domain_with_path(self):
129+
"""Test that function rejects domain-only format with path separator."""
130+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "example.com/path"}):
131+
result = get_validated_domain_override()
132+
self.assertIsNone(result)
133+
134+
def test_rejects_protocol_without_hostname(self):
135+
"""Test that function rejects URLs with protocol but no hostname."""
136+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "https://"}):
137+
result = get_validated_domain_override()
138+
self.assertIsNone(result)
139+
140+
def test_rejects_malformed_url_http_colon(self):
141+
"""Test that function rejects malformed URLs like 'http:8080' (missing slashes)."""
142+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "http:8080"}):
143+
result = get_validated_domain_override()
144+
self.assertIsNone(result)
145+
146+
def test_rejects_malformed_url_https_colon(self):
147+
"""Test that function rejects malformed URLs like 'https:443' (missing slashes)."""
148+
with patch.dict(os.environ, {"A365_OBSERVABILITY_DOMAIN_OVERRIDE": "https:443"}):
149+
result = get_validated_domain_override()
150+
self.assertIsNone(result)
151+
152+
67153
if __name__ == "__main__":
68154
unittest.main()

‎tests/observability/core/test_agent365_exporter.py‎

Lines changed: 107 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -507,10 +507,115 @@ def test_export_ignores_empty_domain_override(self):
507507
# Verify PowerPlatformApiDiscovery was called (override was ignored)
508508
mock_discovery_class.assert_called_once_with("test")
509509

510+
def test_export_uses_valid_url_override_with_https(self):
511+
"""Test that domain override with https:// protocol is accepted and used correctly."""
512+
# Arrange
513+
os.environ["A365_OBSERVABILITY_DOMAIN_OVERRIDE"] = "https://override.example.com"
514+
515+
# Create exporter after setting environment variable
516+
exporter = _Agent365Exporter(
517+
token_resolver=self.mock_token_resolver, cluster_category="test"
518+
)
519+
520+
spans = [self._create_mock_span("test_span")]
521+
522+
# Mock the PowerPlatformApiDiscovery class (should NOT be called since override is valid)
523+
with patch(
524+
"microsoft_agents_a365.observability.core.exporters.agent365_exporter.PowerPlatformApiDiscovery"
525+
) as mock_discovery_class:
526+
# Mock the _post_with_retries method
527+
with patch.object(exporter, "_post_with_retries", return_value=True) as mock_post:
528+
# Act
529+
result = exporter.export(spans)
530+
531+
# Assert
532+
self.assertEqual(result, SpanExportResult.SUCCESS)
533+
mock_post.assert_called_once()
534+
535+
# Verify the call arguments - should use override URL without duplicating protocol
536+
args, kwargs = mock_post.call_args
537+
url, body, headers = args
538+
539+
expected_url = "https://override.example.com/maven/agent365/agents/test-agent-456/traces?api-version=1"
540+
self.assertEqual(url, expected_url)
541+
542+
# Verify PowerPlatformApiDiscovery was not called
543+
mock_discovery_class.assert_not_called()
544+
545+
def test_export_uses_valid_url_override_with_http(self):
546+
"""Test that domain override with http:// protocol is accepted and used correctly."""
547+
# Arrange
548+
os.environ["A365_OBSERVABILITY_DOMAIN_OVERRIDE"] = "http://localhost:8080"
549+
550+
# Create exporter after setting environment variable
551+
exporter = _Agent365Exporter(
552+
token_resolver=self.mock_token_resolver, cluster_category="test"
553+
)
554+
555+
spans = [self._create_mock_span("test_span")]
556+
557+
# Mock the PowerPlatformApiDiscovery class (should NOT be called since override is valid)
558+
with patch(
559+
"microsoft_agents_a365.observability.core.exporters.agent365_exporter.PowerPlatformApiDiscovery"
560+
) as mock_discovery_class:
561+
# Mock the _post_with_retries method
562+
with patch.object(exporter, "_post_with_retries", return_value=True) as mock_post:
563+
# Act
564+
result = exporter.export(spans)
565+
566+
# Assert
567+
self.assertEqual(result, SpanExportResult.SUCCESS)
568+
mock_post.assert_called_once()
569+
570+
# Verify the call arguments - should use override URL with http protocol
571+
args, kwargs = mock_post.call_args
572+
url, body, headers = args
573+
574+
expected_url = "http://localhost:8080/maven/agent365/agents/test-agent-456/traces?api-version=1"
575+
self.assertEqual(url, expected_url)
576+
577+
# Verify PowerPlatformApiDiscovery was not called
578+
mock_discovery_class.assert_not_called()
579+
580+
def test_export_uses_valid_domain_override_with_port(self):
581+
"""Test that domain override with port (no protocol) is accepted and https:// is prepended."""
582+
# Arrange
583+
os.environ["A365_OBSERVABILITY_DOMAIN_OVERRIDE"] = "example.com:8080"
584+
585+
# Create exporter after setting environment variable
586+
exporter = _Agent365Exporter(
587+
token_resolver=self.mock_token_resolver, cluster_category="test"
588+
)
589+
590+
spans = [self._create_mock_span("test_span")]
591+
592+
# Mock the PowerPlatformApiDiscovery class (should NOT be called since override is valid)
593+
with patch(
594+
"microsoft_agents_a365.observability.core.exporters.agent365_exporter.PowerPlatformApiDiscovery"
595+
) as mock_discovery_class:
596+
# Mock the _post_with_retries method
597+
with patch.object(exporter, "_post_with_retries", return_value=True) as mock_post:
598+
# Act
599+
result = exporter.export(spans)
600+
601+
# Assert
602+
self.assertEqual(result, SpanExportResult.SUCCESS)
603+
mock_post.assert_called_once()
604+
605+
# Verify the call arguments - should prepend https:// to domain with port
606+
args, kwargs = mock_post.call_args
607+
url, body, headers = args
608+
609+
expected_url = "https://example.com:8080/maven/agent365/agents/test-agent-456/traces?api-version=1"
610+
self.assertEqual(url, expected_url)
611+
612+
# Verify PowerPlatformApiDiscovery was not called
613+
mock_discovery_class.assert_not_called()
614+
510615
def test_export_ignores_invalid_domain_with_protocol(self):
511-
"""Test that domain override containing protocol is ignored."""
616+
"""Test that domain override with invalid protocol is ignored."""
512617
# Arrange
513-
os.environ["A365_OBSERVABILITY_DOMAIN_OVERRIDE"] = "https://invalid.example.com"
618+
os.environ["A365_OBSERVABILITY_DOMAIN_OVERRIDE"] = "ftp://invalid.example.com"
514619

515620
# Create exporter after setting environment variable
516621
exporter = _Agent365Exporter(

0 commit comments

Comments
 (0)