Add Internal URLRequest Validation API (#2905)

* Add URLRequest validation, error, and tests.

* Clean up whitespace.

Source/AFError.swift

@@ -27,29 +27,6 @@ import Foundation
 /// `AFError` is the error type returned by Alamofire. It encompasses a few different types of errors, each with
 /// their own associated reasons.
 public enum AFError: Error {
-    /// The underlying reason the `.parameterEncodingFailed` error occurred.
-    public enum ParameterEncodingFailureReason {
-        /// The `URLRequest` did not have a `URL` to encode.
-        case missingURL
-        /// JSON serialization failed with an underlying system error during the encoding process.
-        case jsonEncodingFailed(error: Error)
-    }
-
-    /// The underlying reason the `.parameterEncoderFailed` error occurred.
-    public enum ParameterEncoderFailureReason {
-        /// Possible missing components.
-        public enum RequiredComponent {
-            /// The `URL` was missing or unable to be extracted from the passed `URLRequest` or during encoding.
-            case url
-            /// The `HTTPMethod` could not be extracted from the passed `URLRequest`.
-            case httpMethod(rawValue: String)
-        }
-        /// A `RequiredComponent` was missing during encoding.
-        case missingRequiredComponent(RequiredComponent)
-        /// The underlying encoder failed with the associated error.
-        case encoderFailed(error: Error)
-    }
-
     /// The underlying reason the `.multipartEncodingFailed` error occurred.
     public enum MultipartEncodingFailureReason {
         /// The `fileURL` provided for reading an encodable body part isn't a file `URL`.
@@ -80,6 +57,29 @@ public enum AFError: Error {
         case inputStreamReadFailed(error: Error)
     }
 
+    /// The underlying reason the `.parameterEncodingFailed` error occurred.
+    public enum ParameterEncodingFailureReason {
+        /// The `URLRequest` did not have a `URL` to encode.
+        case missingURL
+        /// JSON serialization failed with an underlying system error during the encoding process.
+        case jsonEncodingFailed(error: Error)
+    }
+
+    /// The underlying reason the `.parameterEncoderFailed` error occurred.
+    public enum ParameterEncoderFailureReason {
+        /// Possible missing components.
+        public enum RequiredComponent {
+            /// The `URL` was missing or unable to be extracted from the passed `URLRequest` or during encoding.
+            case url
+            /// The `HTTPMethod` could not be extracted from the passed `URLRequest`.
+            case httpMethod(rawValue: String)
+        }
+        /// A `RequiredComponent` was missing during encoding.
+        case missingRequiredComponent(RequiredComponent)
+        /// The underlying encoder failed with the associated error.
+        case encoderFailed(error: Error)
+    }
+
     /// The underlying reason the `.responseValidationFailed` error occurred.
     public enum ResponseValidationFailureReason {
         /// The data file containing the server response did not exist.
@@ -158,30 +158,37 @@ public enum AFError: Error {
         case publicKeyPinningFailed(host: String, trust: SecTrust, pinnedKeys: [SecKey], serverKeys: [SecKey])
     }
 
-    /// `Session` which issued the `Request` was deinitialized, most likely because its reference went out of scope.
-    case sessionDeinitialized
-    /// `Session` was explicitly invalidated, possibly with the `Error` produced by the underlying `URLSession`.
-    case sessionInvalidated(error: Error?)
+    /// The underlying reason the `.urlRequestValidationFailed`
+    public enum URLRequestValidationFailureReason {
+        case bodyDataInGETRequest(Data)
+    }
+
     /// `Request` was explicitly cancelled.
     case explicitlyCancelled
     /// `URLConvertible` type failed to create a valid `URL`.
     case invalidURL(url: URLConvertible)
+    /// Multipart form encoding failed.
+    case multipartEncodingFailed(reason: MultipartEncodingFailureReason)
     /// `ParameterEncoding` threw an error during the encoding process.
     case parameterEncodingFailed(reason: ParameterEncodingFailureReason)
     /// `ParameterEncoder` threw an error while running the encoder.
     case parameterEncoderFailed(reason: ParameterEncoderFailureReason)
-    /// Multipart form encoding failed.
-    case multipartEncodingFailed(reason: MultipartEncodingFailureReason)
     /// `RequestAdapter` failed threw an error during adaptation.
     case requestAdaptationFailed(error: Error)
+    /// `RequestRetrier` threw an error during the request retry process.
+    case requestRetryFailed(retryError: Error, originalError: Error)
     /// Response validation failed.
     case responseValidationFailed(reason: ResponseValidationFailureReason)
-    /// Response serialization failued.
+    /// Response serialization failed.
     case responseSerializationFailed(reason: ResponseSerializationFailureReason)
     /// `ServerTrustEvaluating` instance threw an error during trust evaluation.
     case serverTrustEvaluationFailed(reason: ServerTrustFailureReason)
-    /// `RequestRetrier` threw an error during the request retry process.
-    case requestRetryFailed(retryError: Error, originalError: Error)
+    /// `Session` which issued the `Request` was deinitialized, most likely because its reference went out of scope.
+    case sessionDeinitialized
+    /// `Session` was explicitly invalidated, possibly with the `Error` produced by the underlying `URLSession`.
+    case sessionInvalidated(error: Error?)
+    /// `URLRequest` failed validation.
+    case urlRequestValidationFailed(reason: URLRequestValidationFailureReason)
 }
 
 extension Error {
@@ -427,13 +434,6 @@ extension AFError.ServerTrustFailureReason {
 extension AFError: LocalizedError {
     public var errorDescription: String? {
         switch self {
-        case .sessionDeinitialized:
-            return """
-                   Session was invalidated without error, so it was likely deinitialized unexpectedly. \
-                   Be sure to retain a reference to your Session for the duration of your requests.
-                   """
-        case .sessionInvalidated(let error):
-            return "Session was invalidated with error: \(error?.localizedDescription ?? "No description.")"
         case .explicitlyCancelled:
             return "Request explicitly cancelled."
         case .invalidURL(let url):
@@ -450,13 +450,22 @@ extension AFError: LocalizedError {
             return reason.localizedDescription
         case .responseSerializationFailed(let reason):
             return reason.localizedDescription
-        case .serverTrustEvaluationFailed:
-            return "Server trust evaluation failed."
         case .requestRetryFailed(let retryError, let originalError):
             return """
                    Request retry failed with retry error: \(retryError.localizedDescription), \
                    original error: \(originalError.localizedDescription)
                    """
+        case .sessionDeinitialized:
+            return """
+                   Session was invalidated without error, so it was likely deinitialized unexpectedly. \
+                   Be sure to retain a reference to your Session for the duration of your requests.
+                   """
+        case .sessionInvalidated(let error):
+            return "Session was invalidated with error: \(error?.localizedDescription ?? "No description.")"
+        case .serverTrustEvaluationFailed:
+            return "Server trust evaluation failed."
+        case let .urlRequestValidationFailed(reason):
+            return "URLRequest validation failed due to reason: \(reason.localizedDescription)"
         }
     }
 }
@@ -594,3 +603,15 @@ extension AFError.ServerTrustFailureReason {
         }
     }
 }
+
+extension AFError.URLRequestValidationFailureReason {
+    var localizedDescription: String {
+        switch self {
+        case let .bodyDataInGETRequest(data):
+            return """
+            Invalid URLRequest with a GET method that had body data:
+            \(String(decoding: data, as: UTF8.self))
+            """
+        }
+    }
+}

Source/Session.swift

@@ -861,6 +861,7 @@ open class Session {
     func performSetupOperations(for request: Request, convertible: URLRequestConvertible) {
         do {
             let initialRequest = try convertible.asURLRequest()
+            try initialRequest.validate()
             rootQueue.async { request.didCreateInitialURLRequest(initialRequest) }
 
             guard !request.isCancelled else { return }
@@ -869,6 +870,7 @@ open class Session {
                 adapter.adapt(initialRequest, for: self) { result in
                     do {
                         let adaptedRequest = try result.get()
+                        try adaptedRequest.validate()
 
                         self.rootQueue.async {
                             request.didAdaptInitialRequest(initialRequest, to: adaptedRequest)

Source/URLRequest+Alamofire.swift

@@ -30,4 +30,10 @@ public extension URLRequest {
         get { return httpMethod.flatMap(HTTPMethod.init) }
         set { httpMethod = newValue?.rawValue }
     }
+
+    func validate() throws {
+        if method == .get, let bodyData = httpBody {
+            throw AFError.urlRequestValidationFailed(reason: .bodyDataInGETRequest(bodyData))
+        }
+    }
 }

Tests/AFError+AlamofireTests.swift

@@ -168,6 +168,13 @@ extension AFError {
         if case let .responseValidationFailed(reason) = self, reason.isUnacceptableStatusCode { return true }
         return false
     }
+
+    // URLRequestValidationFailure
+
+    var isBodyDataInGETRequest: Bool {
+        if case let .urlRequestValidationFailed(reason) = self, reason.isBodyDataInGETRequest { return true }
+        return false
+    }
 }
 
 // MARK: -
@@ -374,3 +381,10 @@ extension AFError.ServerTrustFailureReason {
         return false
     }
 }
+
+extension AFError.URLRequestValidationFailureReason {
+    var isBodyDataInGETRequest: Bool {
+        if case .bodyDataInGETRequest = self { return true }
+        return false
+    }
+}

Tests/ResponseSerializationTests.swift

@@ -1413,34 +1413,34 @@ final class DataPreprocessorTests: BaseTestCase {
         // Given
         let preprocessor = PassthroughPreprocessor()
         let data = Data("data".utf8)
-        
+
         // When
         let result = Result { try preprocessor.preprocess(data) }
-        
+
         // Then
         XCTAssertEqual(data, result.value, "Preprocessed data should equal original data.")
     }
-    
+
     func testThatGoogleXSSIPreprocessorProperlyPreprocessesData() {
         // Given
         let preprocessor = GoogleXSSIPreprocessor()
         let data = Data(")]}',\nabcd".utf8)
-        
+
         // When
         let result = Result { try preprocessor.preprocess(data) }
-        
+
         // Then
         XCTAssertEqual(result.value.map { String(decoding: $0, as: UTF8.self) }, "abcd")
     }
-    
+
     func testThatGoogleXSSIPreprocessorDoesNotChangeDataIfPrefixDoesNotMatch() {
         // Given
         let preprocessor = GoogleXSSIPreprocessor()
         let data = Data("abcd".utf8)
-        
+
         // When
         let result = Result { try preprocessor.preprocess(data) }
-        
+
         // Then
         XCTAssertEqual(result.value.map { String(decoding: $0, as: UTF8.self) }, "abcd")
     }

Tests/SessionTests.swift

@@ -1650,6 +1650,57 @@ final class SessionCancellationTestCase: BaseTestCase {
             XCTAssertTrue(session.activeRequests.isEmpty, "activeRequests should be empty but has \(session.activeRequests.count) items")
         }
     }
+
+    func testThatGETRequestsWithBodyDataAreConsideredInvalid() {
+        // Given
+        let session = Session()
+        var request = URLRequest.makeHTTPBinRequest()
+        request.httpBody = Data("invalid".utf8)
+        let expect = expectation(description: "request should complete")
+        var response: DataResponse<HTTPBinResponse>?
+
+        // When
+        session.request(request).responseDecodable(of: HTTPBinResponse.self) { resp in
+            response = resp
+            expect.fulfill()
+        }
+
+        waitForExpectations(timeout: timeout)
+
+        // Then
+        XCTAssertEqual(response?.result.isFailure, true)
+        XCTAssertEqual(response?.error?.asAFError?.isBodyDataInGETRequest, true)
+    }
+
+    func testThatAdaptedGETRequestsWithBodyDataAreConsideredInvalid() {
+        // Given
+        struct InvalidAdapter: RequestInterceptor {
+            func adapt(_ urlRequest: URLRequest,
+                       for session: Session,
+                       completion: @escaping (Result<URLRequest, Error>) -> Void) {
+                var request = urlRequest
+                request.httpBody = Data("invalid".utf8)
+
+                completion(.success(request))
+            }
+        }
+        let session = Session(interceptor: InvalidAdapter())
+        let request = URLRequest.makeHTTPBinRequest()
+        let expect = expectation(description: "request should complete")
+        var response: DataResponse<HTTPBinResponse>?
+
+        // When
+        session.request(request).responseDecodable(of: HTTPBinResponse.self) { resp in
+            response = resp
+            expect.fulfill()
+        }
+
+        waitForExpectations(timeout: timeout)
+
+        // Then
+        XCTAssertEqual(response?.result.isFailure, true)
+        XCTAssertEqual(response?.error?.asAFError?.underlyingError?.asAFError?.isBodyDataInGETRequest, true)
+    }
 }
 
 // MARK: -