fix(API): remove encoding step before feeding request to signer (#2666)

Author: atierian

.github/workflows/integ_test_api.yml

@@ -190,7 +190,6 @@ jobs:
           scheme: AWSAPIPluginRESTUserPoolTests
 
   api-rest-iam-test:
-    if: ${{ false }}
     needs: prepare-for-test
     runs-on: macos-12
     environment: IntegrationTest

AmplifyPlugins/API/Sources/AWSAPIPlugin/Interceptor/RequestInterceptor/IAMURLRequestInterceptor.swift

@@ -26,52 +26,56 @@ struct IAMURLRequestInterceptor: URLRequestInterceptor {
     }
 
     func intercept(_ request: URLRequest) async throws -> URLRequest {
-        guard let mutableRequest = (request as NSURLRequest).mutableCopy() as? NSMutableURLRequest else {
-            throw APIError.unknown("Could not get mutable request", "")
-        }
-        guard let url = mutableRequest.url else {
+        var request = request
+
+        guard let url = request.url else {
             throw APIError.unknown("Could not get url from mutable request", "")
         }
         guard let host = url.host else {
             throw APIError.unknown("Could not get host from mutable request", "")
         }
 
-        mutableRequest.setValue(URLRequestConstants.ContentType.applicationJson, forHTTPHeaderField: URLRequestConstants.Header.contentType)
-        mutableRequest.setValue(host, forHTTPHeaderField: "host")
-        mutableRequest.setValue(AWSAPIPluginsCore.baseUserAgent(), forHTTPHeaderField: URLRequestConstants.Header.userAgent)
+        request.setValue(URLRequestConstants.ContentType.applicationJson, forHTTPHeaderField: URLRequestConstants.Header.contentType)
+        request.setValue(host, forHTTPHeaderField: "host")
+        request.setValue(AWSAPIPluginsCore.baseUserAgent(), forHTTPHeaderField: URLRequestConstants.Header.userAgent)
 
-        let httpMethod = HttpMethodType(rawValue: mutableRequest.httpMethod.uppercased()) ?? .get
+        let httpMethod = (request.httpMethod?.uppercased())
+            .flatMap(HttpMethodType.init(rawValue:)) ?? .get
+
+        let queryItems = URLComponents(url: url, resolvingAgainstBaseURL: false)?.queryItems ?? []
 
         let requestBuilder = SdkHttpRequestBuilder()
             .withHost(host)
             .withPath(url.path)
+            .withQueryItems(queryItems)
             .withMethod(httpMethod)
             .withPort(443)
             .withProtocol(.https)
-            .withHeaders(.init(mutableRequest.allHTTPHeaderFields ?? [:]))
-            .withBody(.data(mutableRequest.httpBody))
+            .withHeaders(.init(request.allHTTPHeaderFields ?? [:]))
+            .withBody(.data(request.httpBody))
 
         let signingName: String
         switch endpointType {
         case .graphQL:
             signingName = URLRequestConstants.appSyncServiceName
         case .rest:
             signingName = URLRequestConstants.apiGatewayServiceName
-
         }
 
-        guard let urlRequest = try await AmplifyAWSSignatureV4Signer().sigV4SignedRequest(requestBuilder: requestBuilder,
-                                                                                    credentialsProvider: iamCredentialsProvider.getCredentialsProvider(),
-                                                                                    signingName: signingName,
-                                                                                    signingRegion: region,
-                                                                                    date: Date()) else {
+        guard let urlRequest = try await AmplifyAWSSignatureV4Signer().sigV4SignedRequest(
+            requestBuilder: requestBuilder,
+            credentialsProvider: iamCredentialsProvider.getCredentialsProvider(),
+            signingName: signingName,
+            signingRegion: region,
+            date: Date()
+        ) else {
             throw APIError.unknown("Unable to sign request", "")
         }
 
         for header in urlRequest.headers.headers {
-            mutableRequest.setValue(header.value.joined(separator: ","), forHTTPHeaderField: header.name)
+            request.setValue(header.value.joined(separator: ","), forHTTPHeaderField: header.name)
         }
 
-        return mutableRequest as URLRequest
+        return request
     }
 }

AmplifyPlugins/API/Sources/AWSAPIPlugin/Operation/AWSRESTOperation.swift

@@ -77,9 +77,11 @@ final public class AWSRESTOperation: AmplifyOperation<
         // Construct URL with path
         let url: URL
         do {
-            url = try RESTOperationRequestUtils.constructURL(for: endpointConfig.baseURL,
-                                                             with: request.path,
-                                                             with: request.queryParameters)
+            url = try RESTOperationRequestUtils.constructURL(
+                for: endpointConfig.baseURL,
+                withPath: request.path,
+                withParams: request.queryParameters
+            )
         } catch let error as APIError {
             dispatch(result: .failure(error))
             finish()

AmplifyPlugins/API/Sources/AWSAPIPlugin/Support/Internal/URLComponents+sigv4Encoding.swift

@@ -9,15 +9,19 @@ import Foundation
 import Amplify
 
 final class RESTOperationRequestUtils {
-    private init() {
+    private init() {}
 
-    }
-
-    static func constructURL(for baseURL: URL,
-                             with path: String?,
-                             with queryParameters: [String: String]?) throws -> URL {
-        guard var components = URLComponents(url: baseURL, resolvingAgainstBaseURL: false) else {
-            throw APIError.invalidURL("Invalid URL: \(baseURL.absoluteString)",
+    static func constructURL(
+        for baseURL: URL,
+        withPath path: String?,
+        withParams queryParameters: [String: String]?
+    ) throws -> URL {
+        guard var components = URLComponents(
+            url: baseURL,
+            resolvingAgainstBaseURL: false
+        ) else {
+            throw APIError.invalidURL(
+                "Invalid URL: \(baseURL.absoluteString)",
                 """
                 Review your API plugin configuration and ensure \(baseURL.absoluteString) is a valid URL for
                 the 'Endpoint' field.
@@ -29,7 +33,9 @@ final class RESTOperationRequestUtils {
             components.path.append(path)
         }
 
-        try components.encodeQueryItemsPerSigV4Rules(queryParameters)
+        if let queryParameters = queryParameters {
+            components.queryItems = try prepareQueryParamsForSigning(params: queryParameters)
+        }
 
         guard let url = components.url else {
             throw APIError.invalidURL(
@@ -63,4 +69,41 @@ final class RESTOperationRequestUtils {
         baseRequest.httpBody = requestPayload
         return baseRequest
     }
+
+    private static let permittedQueryParamCharacters = CharacterSet.alphanumerics
+        .union(.init(charactersIn: "/_-.~"))
+
+    private static func prepareQueryParamsForSigning(params: [String: String]) throws -> [URLQueryItem] {
+        // remove percent encoding to prepare for request signing
+        // `removingPercentEncoding` is a no-op if the query isn't encoded
+        func removePercentEncoding(key: String, value: String) -> (String, String) {
+            (key.removingPercentEncoding ?? key, value.removingPercentEncoding ?? value)
+        }
+
+        // Disallowed characters are checked for in the Swift SDK. However it effectively silently fails
+        // there by removing any invalid parameters. We're conducting this check here to inform the call-
+        // site.
+        func confirmOnlyPermittedCharactersPresent(key: String, value: String) throws -> (String, String) {
+            guard value.rangeOfCharacter(from: permittedQueryParamCharacters) != nil,
+                key.rangeOfCharacter(from: permittedQueryParamCharacters) != nil
+            else {
+                throw APIError.invalidURL(
+                    "Invalid query parameter.",
+                    """
+                    Review your Amplify.API call to make sure you are passing \
+                    valid UTF-8 query parameters in your request.
+                    The value passed was '\(key)=\(value)'
+                    """
+                )
+            }
+            return (key, value)
+        }
+
+        let queryItems = try params
+            .map(removePercentEncoding)
+            .map(confirmOnlyPermittedCharactersPresent)
+            .map(URLQueryItem.init)
+
+        return queryItems
+    }
 }

AmplifyPlugins/API/Sources/AWSAPIPlugin/Support/Utils/RESTOperationRequestUtils.swift

@@ -37,14 +37,6 @@
                BlueprintName = "AWSAPIPluginRESTIAMTests"
                ReferencedContainer = "container:APIHostApp.xcodeproj">
             </BuildableReference>
-            <SkippedTests>
-               <Test
-                  Identifier = "RESTWithIAMIntegrationTests/testGetAPIWithEncodedQueryParamsSuccess()">
-               </Test>
-               <Test
-                  Identifier = "RESTWithIAMIntegrationTests/testGetAPIWithQueryParamsSuccess()">
-               </Test>
-            </SkippedTests>
          </TestableReference>
       </Testables>
    </TestAction>

AmplifyPlugins/API/Tests/APIHostApp/APIHostApp.xcodeproj/xcshareddata/xcschemes/AWSAPIPluginRESTIAMTests.xcscheme

@@ -87,7 +87,6 @@ class RESTWithIAMIntegrationTests: XCTestCase {
     }
 
     func testGetAPIWithEncodedQueryParamsSuccess() async throws {
-        throw XCTSkip("This test is currently failing with 403 and needs to be fixed")
         let request = RESTRequest(path: "/items",
                                   queryParameters: [
                                     "user": "hello%40email.com",
@@ -99,7 +98,6 @@ class RESTWithIAMIntegrationTests: XCTestCase {
     }
 
     func testPutAPISuccess() async throws {
-        throw XCTSkip("This test is currently failing with 403 and needs to be fixed")
         let request = RESTRequest(path: "/items")
         let data = try await Amplify.API.put(request: request)
         let result = String(decoding: data, as: UTF8.self)