Encode application/x-www-form-urlencoded better

`application/x-www-form-urlencoded` defines `+` as meaning a space. All
servers that accept this content type should interpret it that way, and
most servers also interpret query strings the exact same way.
Unfortunately, we weren't encoding `+` at all. And it turns out that
`URLComponents.queryItems` also doesn't treat `+` specially.

In the interests of maximum compatibility, we actually convert spaces to
`%20` instead of to `+`.

README.md

@@ -405,6 +405,7 @@ Unless you explicitly state otherwise, any contribution intentionally submitted
 #### Development
 
 * Add more Obj-C request constructors.
+* Fix encoding of `+` characters in query strings and `application/x-www-form-urlencoded` bodies.
 
 #### v1.0.3 (2016-09-23)
 

Sources/HTTPManager.swift

@@ -1123,7 +1123,7 @@ extension HTTPManager {
         var urlRequest = request._preparedURLRequest
         var uploadBody = uploadBody
         if case .formUrlEncoded(let queryItems)? = uploadBody {
-            uploadBody = .data(UploadBody.dataRepresentationForQueryItems(queryItems))
+            uploadBody = .data(FormURLEncoded.data(for: queryItems))
         }
         uploadBody?.evaluatePending()
         if let mock = request.mock ?? mockManager.mockForRequest(urlRequest, environment: environment) {
@@ -1411,7 +1411,7 @@ extension SessionDelegate: URLSessionDataDelegate {
                     self.log("providing stream for FormUrlEncoded")
                 #endif
                 autoreleasepool {
-                    completionHandler(InputStream(data: UploadBody.dataRepresentationForQueryItems(queryItems)))
+                    completionHandler(InputStream(data: FormURLEncoded.data(for: queryItems)))
                 }
             }
         case .json(let json)?:

Sources/HTTPManagerRequest.swift

@@ -48,11 +48,12 @@ public class HTTPManagerRequest: NSObject, NSCopying {
         guard var comps = URLComponents(url: baseURL, resolvingAgainstBaseURL: false) else {
             fatalError("HTTPManager: base URL cannot be parsed by NSURLComponents: \(baseURL.relativeString)")
         }
-        if var queryItems = comps.queryItems {
-            queryItems.append(contentsOf: parameters)
-            comps.queryItems = queryItems
+        if var query = comps.percentEncodedQuery, !query.isEmpty {
+            query += "&"
+            query += FormURLEncoded.string(for: parameters)
+            comps.percentEncodedQuery = query
         } else {
-            comps.queryItems = parameters
+            comps.percentEncodedQuery = FormURLEncoded.string(for: parameters)
         }
         return comps.url(relativeTo: baseURL.baseURL)!
     }
@@ -485,7 +486,7 @@ public class HTTPManagerNetworkRequest: HTTPManagerRequest, HTTPManagerRequestPe
         case .data(let data)?:
             request.httpBody = data
         case .formUrlEncoded(let queryItems)?:
-            request.httpBody = UploadBody.dataRepresentationForQueryItems(queryItems)
+            request.httpBody = FormURLEncoded.data(for: queryItems)
         case .json(let json)?:
             request.httpBody = JSON.encodeAsData(json, pretty: false)
         case let .multipartMixed(boundary, parameters, bodyParts)?:

Sources/UploadSupport.swift

@@ -15,32 +15,45 @@
 import Foundation
 import PMJSON
 
-internal enum UploadBody {
-    case data(Data)
-    case formUrlEncoded([URLQueryItem])
-    case json(PMJSON.JSON)
-    /// - Requires: The `boundary` must meet the rules for a valid multipart boundary
-    ///   and must not contain any characters that require quoting.
-    case multipartMixed(boundary: String, parameters: [URLQueryItem], bodyParts: [MultipartBodyPart])
-
-    static func dataRepresentationForQueryItems(_ queryItems: [URLQueryItem]) -> Data {
+/// Implements application/x-www-form-urlencoded encoding.
+///
+/// See [the spec](https://www.w3.org/TR/html5/forms.html#application/x-www-form-urlencoded-encoding-algorithm) for details.
+///
+/// - Note: We don't actually translate spaces into `+` because, while HTML says that query strings
+///   should be encoded using application/x-www-form-urlencoded (and therefore spaces should be `+`),
+///   there's no actual guarantee that servers interpret query strings that way, but all servers are
+///   guaranteed to interpret `%20` as a space.
+internal enum FormURLEncoded {
+    static func string(for queryItems: [URLQueryItem]) -> String {
         guard !queryItems.isEmpty else {
-            return Data()
+            return ""
         }
-        let cs = CharacterSet.urlQueryKeyValueAllowedCharacters
-        func encodeQueryItem(_ item: URLQueryItem) -> String {
-            let encodedName = item.name.addingPercentEncoding(withAllowedCharacters: cs) ?? ""
+        // NB: don't use lazy here, or `joined(separator:)` will map all items twice.
+        // Better to have one array allocation than unnecessarily re-encoding all strings.
+        let encodedQueryItems = queryItems.map({ item -> String in
+            let encodedName = item.name.addingPercentEncoding(withAllowedCharacters: .urlQueryKeyAllowedCharacters) ?? ""
             if let value = item.value {
-                let encodedValue = value.addingPercentEncoding(withAllowedCharacters: cs) ?? ""
+                let encodedValue = value.addingPercentEncoding(withAllowedCharacters: .urlQueryValueAllowedCharacters) ?? ""
                 return "\(encodedName)=\(encodedValue)"
             } else {
                 return encodedName
             }
-        }
-        let encodedQueryItems = queryItems.map(encodeQueryItem)
-        let encodedString = encodedQueryItems.joined(separator: "&")
-        return encodedString.data(using: String.Encoding.utf8)!
+        })
+        return encodedQueryItems.joined(separator: "&")
+    }
+
+    static func data(for queryItems: [URLQueryItem]) -> Data {
+        return string(for: queryItems).data(using: .utf8) ?? Data()
     }
+}
+
+internal enum UploadBody {
+    case data(Data)
+    case formUrlEncoded([URLQueryItem])
+    case json(PMJSON.JSON)
+    /// - Requires: The `boundary` must meet the rules for a valid multipart boundary
+    ///   and must not contain any characters that require quoting.
+    case multipartMixed(boundary: String, parameters: [URLQueryItem], bodyParts: [MultipartBodyPart])
 
     /// Calls `evaluate()` on every pending multipart body.
     internal func evaluatePending() {
@@ -52,11 +65,28 @@ internal enum UploadBody {
 }
 
 private extension CharacterSet {
-    static let urlQueryKeyValueAllowedCharacters: CharacterSet = {
+    static let urlQueryKeyAllowedCharacters: CharacterSet = {
         var cs = CharacterSet.urlQueryAllowed
-        cs.remove(charactersIn: "&=")
+        cs.remove(charactersIn: "&=+")
+        cs.makeImmutable()
         return cs
     }()
+
+    static let urlQueryValueAllowedCharacters: CharacterSet = {
+        var cs = CharacterSet.urlQueryAllowed
+        cs.remove(charactersIn: "&+")
+        cs.makeImmutable()
+        return cs
+    }()
+
+    /// Ensures the wrapped character set is immutable.
+    ///
+    /// Mutable character sets are less efficient than immutable ones.
+    private mutating func makeImmutable() {
+        // The only way to ensure this right now is to bridge through Obj-C.
+        // There's also no way to test if we're mutable or not due to the _SwiftNativeNS* wrapper.
+        self = unsafeDowncast((self as NSCharacterSet).copy() as AnyObject, to: NSCharacterSet.self) as CharacterSet
+    }
 }
 
 internal enum MultipartBodyPart {

Tests/PMHTTPTests.swift

@@ -344,6 +344,42 @@ final class PMHTTPTests: PMHTTPTestCase {
         }
     }
 
+    func testQueryItemEdgeCases() {
+        // https://www.w3.org/TR/html5/forms.html#application/x-www-form-urlencoded-encoding-algorithm
+        // x-www-form-urlencoded encoding converts spaces to `+`.
+        // URLQueryItem doesn't handle this conversion, and not all servers expect it.
+        // But because some do, we can't include `+` unescaped.
+        // And of course `&` and `=` need to be encoded.
+
+        let queryItems = [URLQueryItem(name: "foo", value: "bar"), URLQueryItem(name: "key", value: "value + space"), URLQueryItem(name: " +&=", value: " +&=")]
+        let encodedQuery = "foo=bar&key=value%20%2b%20space&%20%2b%26%3d=%20%2b%26="
+
+        // GET
+        expectationForHTTPRequest(httpServer, path: "/foo") { (request, completionHandler) in
+            let query = request.urlComponents.percentEncodedQuery?.lowercased()
+            XCTAssertEqual(query, encodedQuery, "request query string")
+            completionHandler(HTTPServer.Response(status: .ok))
+        }
+        expectationForRequestSuccess(HTTP.request(GET: "foo", parameters: queryItems))
+        waitForExpectations(timeout: 5, handler: nil)
+
+        // POST
+        expectationForHTTPRequest(httpServer, path: "/foo") { (request, completionHandler) in
+            guard request.headers["Content-Type"] == "application/x-www-form-urlencoded" else {
+                XCTFail("Unexpected content type: \(String(reflecting: request.headers["Content-Type"]))")
+                return completionHandler(HTTPServer.Response(status: .unsupportedMediaType))
+            }
+            guard let bodyText = request.body.flatMap({String(data: $0, encoding: String.Encoding.utf8)}) else {
+                XCTFail("Missing request body, or body not utf-8")
+                return completionHandler(HTTPServer.Response(status: .badRequest))
+            }
+            XCTAssertEqual(bodyText.lowercased(), encodedQuery, "request body")
+            completionHandler(HTTPServer.Response(status: .ok))
+        }
+        expectationForRequestSuccess(HTTP.request(POST: "foo", parameters: queryItems))
+        waitForExpectations(timeout: 5, handler: nil)
+    }
+
     func testParse() {
         // parseAsJSON
         expectationForHTTPRequest(httpServer, path: "/foo") { request, completionHandler in