diff --git a/dotnet/src/Functions/Functions.OpenApi/Model/RestApiOperation.cs b/dotnet/src/Functions/Functions.OpenApi/Model/RestApiOperation.cs index 4f4cd2a48948..fc34fe8ea346 100644 --- a/dotnet/src/Functions/Functions.OpenApi/Model/RestApiOperation.cs +++ b/dotnet/src/Functions/Functions.OpenApi/Model/RestApiOperation.cs @@ -168,7 +168,42 @@ internal Uri BuildOperationUrl(IDictionary arguments, Uri? serv var path = this.BuildPath(this.Path, arguments); - return new Uri(serverUrl, $"{path.TrimStart('/')}"); + var url = new Uri(serverUrl, $"{path.TrimStart('/')}"); + + EnsureRequestTargetMatchesServer(serverUrl, url); + + return url; + } + + /// + /// Verifies that URI construction did not move the request off the configured server. A selected + /// operation path must resolve to a request on the same scheme, host, and port and within the + /// server's base path. Otherwise an absolute or authority-changing operation path (for example + /// "https://another-host/admin") could redirect a credential-bearing request to an unintended + /// target even though it carries no dot-segment. This complements + /// so operation selection, path validation, and request construction share one canonical target. + /// + /// The configured server URL. + /// The request URL produced by combining the server URL and operation path. + private static void EnsureRequestTargetMatchesServer(Uri serverUrl, Uri requestUrl) + { + var serverAuthority = serverUrl.GetLeftPart(UriPartial.Authority); + var requestAuthority = requestUrl.GetLeftPart(UriPartial.Authority); + + if (!string.Equals(serverAuthority, requestAuthority, StringComparison.OrdinalIgnoreCase)) + { + throw new KernelException($"The operation path resolves to '{requestAuthority}', which does not match the configured server '{serverAuthority}'."); + } + + // GetServerUrl guarantees a trailing slash, so the server's base path always ends with '/'. + var basePath = serverUrl.AbsolutePath; + var requestPath = requestUrl.AbsolutePath; + + if (!string.Equals(requestPath, basePath.TrimEnd('/'), StringComparison.Ordinal) && + !requestPath.StartsWith(basePath, StringComparison.Ordinal)) + { + throw new KernelException($"The operation path resolves to '{requestPath}', which is outside the configured server base path '{basePath}'."); + } } /// @@ -420,6 +455,27 @@ value is string { } strValue && /// The path to validate. private static void ValidatePathSegments(string path) { + if (ContainsDotSegment(path)) + { + throw new KernelException($"Path '{path}' contains a dot-segment, which could lead to path traversal."); + } + } + + /// + /// Determines whether the supplied path contains a dot-segment (. or ..), including percent-encoded + /// forms (e.g. "%2e%2e") that canonicalizes at request time. This is used both to + /// reject such paths when building a request URL and to exclude them during operation selection so an + /// encoded dot-segment cannot bypass an include/exclude operation-selection filter. + /// + /// The path to inspect. + /// if the path contains a dot-segment; otherwise, . + internal static bool ContainsDotSegment(string path) + { + if (string.IsNullOrEmpty(path)) + { + return false; + } + // Split on the structural path separator first. foreach (var rawSegment in path.Split('/')) { @@ -443,10 +499,12 @@ private static void ValidatePathSegments(string path) { if (segment == "." || segment == "..") { - throw new KernelException($"Path '{path}' contains a dot-segment, which could lead to path traversal."); + return true; } } } + + return false; } private IDictionary _extensions = s_emptyDictionary; diff --git a/dotnet/src/Functions/Functions.OpenApi/OpenApi/OpenApiDocumentParser.cs b/dotnet/src/Functions/Functions.OpenApi/OpenApi/OpenApiDocumentParser.cs index 2db2bf8ae18e..27cf837b0dbc 100644 --- a/dotnet/src/Functions/Functions.OpenApi/OpenApi/OpenApiDocumentParser.cs +++ b/dotnet/src/Functions/Functions.OpenApi/OpenApi/OpenApiDocumentParser.cs @@ -197,6 +197,18 @@ internal static List CreateRestApiOperations(OpenApiDocument d var operationItem = operationPair.Value; var operationServers = CreateRestApiOperationServers(operationItem.Servers); + // Exclude operations whose path contains an encoded or literal dot-segment before the + // selection predicate is consulted. Such a path is canonicalized into a different + // effective endpoint at request time, so offering it to an include/exclude predicate + // would let an encoded dot-segment bypass the operation-selection filter. Absolute or + // authority-changing path keys are already rejected by the OpenAPI reader, so a + // dot-segment check is sufficient here. + if (RestApiOperation.ContainsDotSegment(path)) + { + logger.LogWarning("Operation '{OperationId}' at path '{Path}' contains a dot-segment and will be skipped during operation selection.", operationItem.OperationId, path); + continue; + } + // Skip the operation parsing and don't add it to the result operations list if it's explicitly excluded by the predicate. if (!options?.OperationSelectionPredicate?.Invoke(new OperationSelectionPredicateContext(operationItem.OperationId, path, method, operationItem.Description)) ?? false) { diff --git a/dotnet/src/Functions/Functions.UnitTests/OpenApi/OpenApiDocumentParserV20Tests.cs b/dotnet/src/Functions/Functions.UnitTests/OpenApi/OpenApiDocumentParserV20Tests.cs index 40934fa4ff73..fe91d91e3365 100644 --- a/dotnet/src/Functions/Functions.UnitTests/OpenApi/OpenApiDocumentParserV20Tests.cs +++ b/dotnet/src/Functions/Functions.UnitTests/OpenApi/OpenApiDocumentParserV20Tests.cs @@ -438,6 +438,109 @@ public async Task ItCanFilterOutSpecifiedOperationsAsync() Assert.Contains(restApiSpec.Operations, o => o.Id == "GetSecret"); } + [Fact] + public async Task ItShouldNotOfferEncodedDotSegmentOperationsToSelectionPredicateAsync() + { + // Arrange - one regular operation and one whose path contains an encoded dot-segment + // ("%2e%2e") that canonicalizes to a path-traversal at request time. The selection + // predicate must compare a normalized value, so the encoded path cannot bypass an + // allow/deny operation-selection filter. + var document = + """ + { + "swagger": "2.0", + "info": { + "title": "Test API", + "version": "1.0.0" + }, + "paths": { + "/items": { + "get": { + "operationId": "GetItems", + "summary": "Get items", + "responses": { + "200": { + "description": "Successful response" + } + } + } + }, + "/resources/%2e%2e/admin": { + "get": { + "operationId": "GetAdmin", + "summary": "Get admin", + "responses": { + "200": { + "description": "Successful response" + } + } + } + } + } + } + """; + + var observedPaths = new List(); + var options = new OpenApiDocumentParserOptions + { + OperationSelectionPredicate = (context) => + { + observedPaths.Add(context.Path); + return true; + } + }; + + await using var stream = new MemoryStream(Encoding.UTF8.GetBytes(document)); + + // Act + var restApiSpec = await this._sut.ParseAsync(stream, options); + + // Assert - the regular operation is offered to the predicate and imported. + Assert.Contains("/items", observedPaths); + Assert.Contains(restApiSpec.Operations, o => o.Path == "/items"); + + // The encoded dot-segment operation is excluded before the predicate is consulted, + // so it cannot slip past an include/exclude operation-selection filter. + Assert.DoesNotContain("/resources/%2e%2e/admin", observedPaths); + Assert.DoesNotContain(restApiSpec.Operations, o => o.Path == "/resources/%2e%2e/admin"); + } + + [Fact] + public async Task ItShouldRejectDocumentWithNonRelativeOperationPathAsync() + { + // Arrange - an absolute / authority-changing operation path key would resolve to a request off + // the configured server. The OpenAPI reader rejects such a non-relative path key so it can + // never reach operation selection or request construction. + var document = + """ + { + "swagger": "2.0", + "info": { + "title": "Test API", + "version": "1.0.0" + }, + "paths": { + "https://evil.com/admin": { + "get": { + "operationId": "GetAdmin", + "summary": "Get admin", + "responses": { + "200": { + "description": "Successful response" + } + } + } + } + } + } + """; + + await using var stream = new MemoryStream(Encoding.UTF8.GetBytes(document)); + + // Act & Assert - a non-relative operation path key is rejected during parsing. + await Assert.ThrowsAsync(() => this._sut.ParseAsync(stream)); + } + [Fact] public async Task ItCanParsePathItemPathParametersAsync() { diff --git a/dotnet/src/Functions/Functions.UnitTests/OpenApi/RestApiOperationTests.cs b/dotnet/src/Functions/Functions.UnitTests/OpenApi/RestApiOperationTests.cs index 6273c80d494e..6342ee30c7ed 100644 --- a/dotnet/src/Functions/Functions.UnitTests/OpenApi/RestApiOperationTests.cs +++ b/dotnet/src/Functions/Functions.UnitTests/OpenApi/RestApiOperationTests.cs @@ -1553,6 +1553,86 @@ public void ItShouldAllowEncodedNonDotSegmentCharactersInPathTemplate() Assert.Equal("https://example.com/api/resources/a%20b/details", url.OriginalString); } + [Theory] + [InlineData("https://evil.com/admin")] + [InlineData("https://example.com:8443/admin")] + [InlineData("http://example.com/admin")] + [InlineData(@"\\evil.com\admin")] + [InlineData("https://user:pass@example.com/api/admin")] + [InlineData("https://user@example.com/api/data")] + [InlineData("https://example.com@evil.com/admin")] + public void ItShouldRejectOperationPathThatChangesRequestAuthority(string path) + { + // Arrange — an operation path that is an absolute URI (or otherwise changes the scheme, + // host, or port) resolves to a request off the configured server after URI construction. + // Such a path must be rejected so a credential-bearing request cannot be redirected to an + // unintended target even though it carries no dot-segment. + var sut = new RestApiOperation( + id: "fake_id", + servers: [new RestApiServer("https://example.com/api")], + path: path, + method: HttpMethod.Get, + description: "fake_description", + parameters: [], + responses: new Dictionary(), + securityRequirements: [] + ); + + var arguments = new Dictionary(); + + // Act & Assert — the resolved request target must stay on the configured server. + var ex = Assert.Throws(() => sut.BuildOperationUrl(arguments)); + Assert.Contains("does not match the configured server", ex.Message); + } + + [Fact] + public void ItShouldRejectSameAuthorityBasePathEscape() + { + // Arrange — an absolute path on the same authority but outside the configured base path must + // be rejected, so a request cannot be moved to a different route even when the scheme, host, + // and port all match the configured server. + var sut = new RestApiOperation( + id: "fake_id", + servers: [new RestApiServer("https://example.com/api")], + path: "https://example.com/other/admin", + method: HttpMethod.Get, + description: "fake_description", + parameters: [], + responses: new Dictionary(), + securityRequirements: [] + ); + + var arguments = new Dictionary(); + + // Act & Assert — the resolved request path must remain within the configured server base path. + var ex = Assert.Throws(() => sut.BuildOperationUrl(arguments)); + Assert.Contains("outside the configured server base path", ex.Message); + } + + [Fact] + public void ItShouldBuildUrlForOperationPathOnTheConfiguredServer() + { + // Arrange — a normal relative operation path on the configured server. + var sut = new RestApiOperation( + id: "fake_id", + servers: [new RestApiServer("https://example.com/api")], + path: "/resources/item", + method: HttpMethod.Get, + description: "fake_description", + parameters: [], + responses: new Dictionary(), + securityRequirements: [] + ); + + var arguments = new Dictionary(); + + // Act + var url = sut.BuildOperationUrl(arguments); + + // Assert — the reconciliation must not reject a legitimate same-server path. + Assert.Equal("https://example.com/api/resources/item", url.OriginalString); + } + [Fact] public void ItShouldEncodeServerVariableValuesLookedUpByArgumentName() { diff --git a/python/semantic_kernel/connectors/openapi_plugin/models/rest_api_operation.py b/python/semantic_kernel/connectors/openapi_plugin/models/rest_api_operation.py index 94480a81ae43..4905a7c2102f 100644 --- a/python/semantic_kernel/connectors/openapi_plugin/models/rest_api_operation.py +++ b/python/semantic_kernel/connectors/openapi_plugin/models/rest_api_operation.py @@ -224,9 +224,45 @@ def build_operation_url(self, arguments, server_url_override=None, api_host_url= server_url = self.get_server_url(server_url_override, api_host_url, arguments) path = self.build_path(self.path, arguments) try: - return urljoin(server_url, path.lstrip("/")) + request_url = urljoin(server_url, path.lstrip("/")) except Exception as e: raise FunctionExecutionException(f"Error building the URL for the operation {self.id}: {e!s}") from e + self._ensure_request_target_matches_server(server_url, request_url) + return request_url + + @staticmethod + def _ensure_request_target_matches_server(server_url: str, request_url: str) -> None: + """Verify URL construction did not move the request off the configured server. + + A selected operation path must resolve to a request on the same scheme, host, and port and + within the server's base path. Otherwise an absolute or authority-changing operation path + (for example "https://another-host/admin") could redirect a credential-bearing request to an + unintended target even though it carries no dot-segment. This complements + `_validate_path_segments` so operation selection, path validation, and request construction + share one canonical target. + """ + server = urlparse(server_url) + request = urlparse(request_url) + + if (request.scheme, request.username, request.password, request.hostname, request.port) != ( + server.scheme, + server.username, + server.password, + server.hostname, + server.port, + ): + raise FunctionExecutionException( + f"The operation path resolves to '{request.scheme}://{request.netloc}', which does not match " + f"the configured server '{server.scheme}://{server.netloc}'." + ) + + # get_server_url guarantees a trailing slash, so the server base path always ends with "/". + base_path = server.path + if request.path != base_path.rstrip("/") and not request.path.startswith(base_path): + raise FunctionExecutionException( + f"The operation path resolves to '{request.path}', which is outside the configured server " + f"base path '{base_path}'." + ) def get_server_url(self, server_url_override=None, api_host_url=None, arguments=None): """Get the server URL for the operation.""" @@ -299,6 +335,20 @@ def _validate_path_segments(path: str) -> None: The operation is selected using the raw path but the request URL is built from a canonicalized path, so encoded dot-segments such as "%2e%2e" must be rejected before the URL is constructed. """ + if RestApiOperation._contains_dot_segment(path): + raise FunctionExecutionException( + f"Path '{path}' contains a dot-segment, which could lead to path traversal." + ) + + @staticmethod + def _contains_dot_segment(path: str) -> bool: + """Return True if the path contains a dot-segment (. or ..), including percent-encoded forms. + + Used both to reject such paths when building a request URL and to exclude them during operation + selection so an encoded dot-segment cannot bypass an include/exclude operation-selection filter. + """ + if not path: + return False for segment in path.split("/"): decoded = segment for _ in range(5): @@ -310,9 +360,24 @@ def _validate_path_segments(path: str) -> None: # both "/" and "\" and reject any resulting dot-segment. for part in decoded.replace("\\", "/").split("/"): if part in (".", ".."): - raise FunctionExecutionException( - f"Path '{path}' contains a dot-segment, which could lead to path traversal." - ) + return True + return False + + @staticmethod + def _is_non_relative_path(path: str) -> bool: + """Return True if the path is non-relative (absolute or authority-changing). + + A conformant OpenAPI operation path is relative to the server URL. An absolute URI, a + protocol-relative reference ("//host"), or a backslash-authority reference changes the request + target once combined with the server URL, so such a path is excluded during operation selection + to keep selection and request construction on one canonical target. + """ + if not path: + return False + if path.startswith(("//", "\\\\", "/\\", "\\/")): + return True + parsed = urlparse(path) + return bool(parsed.scheme or parsed.netloc) def build_query_string(self, arguments: dict[str, Any]) -> str: """Build the query string for the operation.""" diff --git a/python/semantic_kernel/connectors/openapi_plugin/openapi_parser.py b/python/semantic_kernel/connectors/openapi_plugin/openapi_parser.py index 1f6aabe5eeff..34bea1bff2dd 100644 --- a/python/semantic_kernel/connectors/openapi_plugin/openapi_parser.py +++ b/python/semantic_kernel/connectors/openapi_plugin/openapi_parser.py @@ -265,6 +265,18 @@ def create_rest_api_operations( summary = details.get("summary", None) description = details.get("description", None) + # Exclude operations whose path would resolve to a different effective request target + # than the one offered to the selection predicate: a dot-segment (encoded or literal) + # or a non-relative (absolute / authority-changing) path. Excluding them here keeps + # operation selection and request construction on one canonical target so such a path + # cannot bypass an include/exclude operation-selection filter. + if RestApiOperation._contains_dot_segment(path) or RestApiOperation._is_non_relative_path(path): + logger.warning( + f"Skipping operation {operationId} at path '{path}' because it does not resolve to a " + f"relative path on the configured server." + ) + continue + context = OperationSelectionPredicateContext(operationId, path, method, description) if ( execution_settings diff --git a/python/tests/unit/connectors/openapi_plugin/test_sk_openapi.py b/python/tests/unit/connectors/openapi_plugin/test_sk_openapi.py index 1b44733562c9..20379131451d 100644 --- a/python/tests/unit/connectors/openapi_plugin/test_sk_openapi.py +++ b/python/tests/unit/connectors/openapi_plugin/test_sk_openapi.py @@ -524,6 +524,44 @@ def test_build_path_allows_encoded_non_dot_segment_characters(): assert operation.build_path(operation.path, {}) == "/resources/a%20b/details" +@pytest.mark.parametrize( + "path", + [ + "https://evil.com/admin", + "https://example.com:8443/admin", + "http://example.com/admin", + "https://user:pass@example.com/api/admin", + "https://user@example.com/api/data", + "https://example.com@evil.com/admin", + ], +) +def test_build_operation_url_rejects_authority_changing_path(path): + # An operation path that is an absolute URI (changing scheme, host, or port) resolves to a + # request off the configured server after URL construction, so a credential-bearing request + # could be redirected to an unintended target even though it carries no dot-segment. + operation = RestApiOperation(id="test", method="GET", servers=["https://example.com/api"], path=path, params=[]) + with pytest.raises(FunctionExecutionException, match="does not match the configured server"): + operation.build_operation_url({}) + + +def test_build_operation_url_allows_same_server_path(): + # A normal relative operation path on the configured server must not be rejected. + operation = RestApiOperation( + id="test", method="GET", servers=["https://example.com/api"], path="/resources/item", params=[] + ) + assert operation.build_operation_url({}) == "https://example.com/api/resources/item" + + +def test_build_operation_url_rejects_same_authority_base_path_escape(): + # An absolute path on the same authority but outside the configured base path must be rejected, + # so a request cannot be moved to a different route even when scheme, host, and port all match. + operation = RestApiOperation( + id="test", method="GET", servers=["https://example.com/api"], path="https://example.com/other/admin", params=[] + ) + with pytest.raises(FunctionExecutionException, match="outside the configured server base path"): + operation.build_operation_url({}) + + def test_build_query_string_with_required_parameter(): parameters = [ RestApiParameter(name="query", type="string", location=RestApiParameterLocation.QUERY, is_required=True) @@ -898,6 +936,93 @@ def test_predicate_callback_applied(openapi_runner_with_predicate_callback): ) +def test_encoded_dot_segment_operations_excluded_from_selection(): + # A regular operation and one whose path contains an encoded dot-segment ("%2e%2e") that + # canonicalizes to a path-traversal at request time. The encoded path must be excluded before + # the selection predicate is consulted so it cannot bypass an include/exclude filter. + parsed_document = { + "servers": [{"url": "https://example.com"}], + "paths": { + "/items": { + "get": { + "operationId": "getItems", + "summary": "Get items", + "responses": {"200": {"description": "OK"}}, + } + }, + "/resources/%2e%2e/admin": { + "get": { + "operationId": "getAdmin", + "summary": "Get admin", + "responses": {"200": {"description": "OK"}}, + } + }, + }, + } + + observed_paths: list[str] = [] + + def predicate_callback(context): + observed_paths.append(context.path) + return True + + parser = OpenApiParser() + exec_settings = OpenAPIFunctionExecutionParameters(operation_selection_predicate=predicate_callback) + operations = parser.create_rest_api_operations(parsed_document, execution_settings=exec_settings) + + # The regular operation is offered to the predicate and imported. + assert "/items" in observed_paths + assert "getItems" in operations + + # The encoded dot-segment operation is excluded before the predicate is consulted, + # so it cannot slip past an include/exclude operation-selection filter. + assert "/resources/%2e%2e/admin" not in observed_paths + assert "getAdmin" not in operations + + +def test_non_relative_path_operations_excluded_from_selection(): + # An operation whose path is an absolute / authority-changing URI resolves to a request off the + # configured server, so it must be excluded before the selection predicate is consulted rather + # than offered to a path-based filter and only rejected later at request construction. + parsed_document = { + "servers": [{"url": "https://example.com"}], + "paths": { + "/items": { + "get": { + "operationId": "getItems", + "summary": "Get items", + "responses": {"200": {"description": "OK"}}, + } + }, + "https://evil.com/admin": { + "get": { + "operationId": "getAdmin", + "summary": "Get admin", + "responses": {"200": {"description": "OK"}}, + } + }, + }, + } + + observed_paths: list[str] = [] + + def predicate_callback(context): + observed_paths.append(context.path) + return True + + parser = OpenApiParser() + exec_settings = OpenAPIFunctionExecutionParameters(operation_selection_predicate=predicate_callback) + operations = parser.create_rest_api_operations(parsed_document, execution_settings=exec_settings) + + # The regular operation is offered to the predicate and imported. + assert "/items" in observed_paths + assert "getItems" in operations + + # The absolute operation path is excluded before the predicate is consulted. + assert "https://evil.com/admin" not in observed_paths + assert "getAdmin" not in operations + + @patch("aiohttp.ClientSession.request") async def test_run_operation_with_invalid_request(mock_request, openapi_runner): runner, operations = openapi_runner