fix: Properly escape HTTP paths for greedy label types (#567)

smithy-lang · Jun 26, 2023 · ee7c8e3 · ee7c8e3
1 parent fa4c779
commit ee7c8e3
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 29 deletions.
diff --git a/Sources/ClientRuntime/Networking/Endpoint.swift b/Sources/ClientRuntime/Networking/Endpoint.swift
@@ -11,7 +11,6 @@ public struct Endpoint: Hashable {
     public let protocolType: ProtocolType?
     public let host: String
     public let port: Int16
-
     public let headers: Headers?
     public let properties: [String: AnyHashable]
 
@@ -67,18 +66,13 @@ extension Endpoint {
         components.host = host
         components.percentEncodedPath = path
         components.percentEncodedQuery = queryItemString
-
         return components.url
     }
 
     var queryItemString: String? {
         guard let queryItems = queryItems else { return nil }
         return queryItems.map { queryItem in
-            if let value = queryItem.value {
-                return "\(queryItem.name)=\(value)"
-            } else {
-                return queryItem.name
-            }
+            return [queryItem.name, queryItem.value].compactMap { $0 }.joined(separator: "=")
         }.joined(separator: "&")
     }
 }
diff --git a/Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift b/Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift
@@ -29,11 +29,10 @@ public class SdkHttpRequest {
 
 extension SdkHttpRequest {
 
-    public func toHttpRequest(escaping: Bool = false) throws -> HTTPRequest {
+    public func toHttpRequest() throws -> HTTPRequest {
         let httpRequest = try HTTPRequest()
         httpRequest.method = method.rawValue
-        let encodedPath = escaping ? endpoint.path.urlPercentEncodedForPath : endpoint.path
-        httpRequest.path = [encodedPath, endpoint.queryItemString].compactMap { $0 }.joined(separator: "?")
+        httpRequest.path = [endpoint.path, endpoint.queryItemString].compactMap { $0 }.joined(separator: "?")
         httpRequest.addHeaders(headers: headers.toHttpHeaders())
         httpRequest.body = StreamableHttpBody(body: body)
         return httpRequest
@@ -42,11 +41,10 @@ extension SdkHttpRequest {
     /// Convert the SDK request to a CRT HTTPRequestBase
     /// CRT converts the HTTPRequestBase to HTTP2Request internally if the protocol is HTTP/2
     /// - Returns: the CRT request
-    public func toHttp2Request(escaping: Bool = false) throws -> HTTPRequestBase {
+    public func toHttp2Request() throws -> HTTPRequestBase {
         let httpRequest = try HTTPRequest()
         httpRequest.method = method.rawValue
-        let encodedPath = escaping ? endpoint.path.urlPercentEncodedForPath : endpoint.path
-        httpRequest.path = [encodedPath, endpoint.queryItemString].compactMap { $0 }.joined(separator: "?")
+        httpRequest.path = [endpoint.path, endpoint.queryItemString].compactMap { $0 }.joined(separator: "?")
         httpRequest.addHeaders(headers: headers.toHttpHeaders())
 
         // HTTP2Request used with manual writes hence we need to set the body to nil

diff --git a/Sources/ClientRuntime/PrimitiveTypeExtensions/String+Extensions.swift b/Sources/ClientRuntime/PrimitiveTypeExtensions/String+Extensions.swift
@@ -100,8 +100,8 @@ extension String {
 }
 
 extension String {
-    public func urlPercentEncoding() -> String {
-        self.urlPercentEncodedForQuery
+    public func urlPercentEncoding(encodeForwardSlash: Bool = true) -> String {
+        encodeForwardSlash ? urlPercentEncodedForQuery : urlPercentEncodedForPath
     }
 }
 

diff --git a/Tests/ClientRuntimeTests/ClientRuntimeTests/MiddlewareTests/AttributesTests.swift b/Tests/ClientRuntimeTests/ClientRuntimeTests/MiddlewareTests/AttributesTests.swift
@@ -21,7 +21,7 @@ class AttributesTests: XCTestCase {
     }
 
     func test_contains_doesNotContainUnsetKey() {
-        var subject = Attributes()
+        let subject = Attributes()
 
         XCTAssertFalse(subject.contains(key: intKey))
     }

diff --git a/Tests/ClientRuntimeTests/NetworkingTests/HttpRequestTests.swift b/Tests/ClientRuntimeTests/NetworkingTests/HttpRequestTests.swift
@@ -15,7 +15,7 @@ class HttpRequestTests: NetworkingTestUtils {
     }
 
     func testSdkHttpRequestToHttpRequest() throws {
-        var headers = Headers(["header-item-name": "header-item-value"])
+        let headers = Headers(["header-item-name": "header-item-value"])
         let endpoint = Endpoint(host: "host.com", path: "/", headers: headers)
 
         let httpBody = HttpBody.data(expectedMockRequestData)
@@ -90,16 +90,7 @@ class HttpRequestTests: NetworkingTestUtils {
         XCTAssert(updatedRequest.queryItems?.contains(ClientRuntime.URLQueryItem(name: "signedthing", value: "signed")) ?? false)
     }
 
-    func testPathInInHttpRequestIsEscapedPerRFC3986() throws {
-        let builder = SdkHttpRequestBuilder()
-            .withHeader(name: "Host", value: "xctest.amazon.com")
-            .withPath("/space /colon:/dollar$/tilde~/dash-/underscore_/period.")
-        let httpRequest = try builder.build().toHttpRequest(escaping: true)
-        let escapedPath = "/space%20/colon%3A/dollar%24/tilde~/dash-/underscore_/period."
-        XCTAssertEqual(httpRequest.path, escapedPath)
-    }
-
-    func testPathInInHttpRequestIsNotEscapedPerRFC3986WhenNotDesired() throws {
+    func testPathInInHttpRequestIsNotAltered() throws {
         let path = "/space /colon:/dollar$/tilde~/dash-/underscore_/period."
         let builder = SdkHttpRequestBuilder()
             .withHeader(name: "Host", value: "xctest.amazon.com")

diff --git a/...ware/amazon/smithy/swift/codegen/integration/middlewares/providers/HttpUrlPathProvider.kt b/...ware/amazon/smithy/swift/codegen/integration/middlewares/providers/HttpUrlPathProvider.kt
@@ -83,14 +83,14 @@ class HttpUrlPathProvider(
                         )
                     }
                     ShapeType.STRING -> {
-                        val percentEncoded = if (!it.isGreedyLabel) ".urlPercentEncoding()" else ""
+                        val percentEncoded = urlEncoding(it.isGreedyLabel)
                         val enumRawValueSuffix =
                             targetShape.getTrait(EnumTrait::class.java).map { ".rawValue" }.orElse("")
                         "$labelMemberName$enumRawValueSuffix$percentEncoded"
                     }
                     ShapeType.FLOAT, ShapeType.DOUBLE -> "$labelMemberName.encoded()"
                     ShapeType.ENUM -> {
-                        val percentEncoded = if (!it.isGreedyLabel) ".urlPercentEncoding()" else ""
+                        val percentEncoded = urlEncoding(it.isGreedyLabel)
                         "$labelMemberName.rawValue$percentEncoded"
                     }
                     else -> labelMemberName
@@ -115,4 +115,20 @@ class HttpUrlPathProvider(
         val uri = resolvedURIComponents.joinToString(separator = "/", prefix = "/", postfix = "")
         writer.write("return \"\$L\"", uri)
     }
+
+    /**
+     * Provides the appropriate Swift method call for URL encoding a String path component.
+     *
+     * For non-greedy labels, forward-slash is encoded because the label must fill in
+     * exactly one path component.
+     *
+     * For greedy labels, forward-slash is not encoded because it is expected that the
+     * label contents will include multiple path components.
+     *
+     * Swift .urlPercentEncoding() method encodes forward slashes by default.
+     */
+    private fun urlEncoding(greedy: Boolean): String {
+        val options = "encodeForwardSlash: false".takeIf { greedy } ?: ""
+        return ".urlPercentEncoding($options)"
+    }
 }