Skip to content

Commit

Permalink
Fix a bug when appending an empty array of form params.
Browse files Browse the repository at this point in the history 
Also makes the logic a little bit clearer and adds tests for empty
key/value names.

Fixes: #140
  • Loading branch information
@karwa
karwa committed Apr 1, 2022
1 parent 186d0b3 commit a675c03
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 32 deletions.
60 changes: 31 additions & 29 deletions Sources/WebURL/WebURL+FormParameters.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,50 +828,52 @@ extension URLStorage {
fromEncoded keyValuePairs: C, lengthIfKnown: Int? = nil
) where C: Collection, C.Element == (UTF8Bytes, UTF8Bytes), UTF8Bytes: Collection, UTF8Bytes.Element == UInt8 {

let encodedKVPsLength: Int
var appendedKVPStringLength: Int
if let knownLength = lengthIfKnown {
encodedKVPsLength = knownLength + (keyValuePairs.count * 2) - 1 /* '=' and '&' */
appendedKVPStringLength = knownLength + (keyValuePairs.count * 2) /* '=' and '&' */ - 1
} else {
encodedKVPsLength =
keyValuePairs.reduce(into: 0) { length, kvp in
length += kvp.0.count + 1 /* '=' */ + kvp.1.count + 1 /* '&' */
} - 1
}

var separator: ASCII?
switch structure.queryLength {
case 0:
// No query. We need to add a "?" delimiter.
separator = .questionMark
case 1:
// There is a query, but it's a lone "?" with no string after it.
separator = nil
default:
// There is a query, and we need to add a "&" between the existing contents and appended KVPs.
separator = .ampersand
appendedKVPStringLength = keyValuePairs.reduce(into: 0) { length, kvp in
length += kvp.0.count + 1 /* '=' */ + kvp.1.count + 1 /* '&' */
}
appendedKVPStringLength -= 1
}

guard let bytesToAppend = URLStorage.SizeType(exactly: encodedKVPsLength + (separator == nil ? 0 : 1)) else {
fatalError(URLSetterError.exceedsMaximumSize.description)
guard appendedKVPStringLength > 0 else {
return
}

// Calculate the new structure and replace the code-units.
var newStructure = structure
newStructure.queryLength += bytesToAppend

let bytesToWrite = URLStorage.SizeType(appendedKVPStringLength) + 1
let insertPosition: URLStorage.SizeType
let delimiter: ASCII

if structure.queryLength <= 1 {
// Replace the entire query.
(insertPosition, delimiter) = (structure.queryStart, .questionMark)
newStructure.queryLength = bytesToWrite
} else {
// Append the KVP string to an existing query.
(insertPosition, delimiter) = (structure.fragmentStart, .ampersand)
newStructure.queryLength += bytesToWrite
}

try! replaceSubrange(
structure.fragmentStart..<structure.fragmentStart,
withUninitializedSpace: bytesToAppend,
insertPosition..<structure.fragmentStart,
withUninitializedSpace: bytesToWrite,
newStructure: newStructure
) { buffer in
var bytesWritten = 0
if let separator = separator {
buffer[0] = separator.codePoint
bytesWritten += 1
}

buffer[0] = delimiter.codePoint
var bytesWritten = 1

for (key, value) in keyValuePairs {
bytesWritten += UnsafeMutableBufferPointer(rebasing: buffer[bytesWritten...]).fastInitialize(from: key)
precondition(bytesWritten < buffer.count, "Invalid collection: contents have changed between iterations")
buffer[bytesWritten] = ASCII.equalSign.codePoint
bytesWritten += 1
precondition(bytesWritten <= buffer.count, "Invalid collection: contents have changed between iterations")
bytesWritten += UnsafeMutableBufferPointer(rebasing: buffer[bytesWritten...]).fastInitialize(from: value)
if bytesWritten < buffer.count {
buffer[bytesWritten] = ASCII.ampersand.codePoint
Expand Down
95 changes: 92 additions & 3 deletions Tests/WebURLTests/FormParametersTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ final class FormEncodedQueryParametersTests: XCTestCase {
// 'contains' returns the same information.
XCTAssertTrue(url.formParams.contains("a"))
XCTAssertTrue(url.formParams.contains("c is the key"))
XCTAssertTrue(url.formParams.contains("e"))
XCTAssertTrue(url.formParams.contains(""))
XCTAssertFalse(url.formParams.contains("doesNotExist"))

Expand Down Expand Up @@ -163,6 +164,24 @@ final class FormEncodedQueryParametersTests: XCTestCase {
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}

// But if there is an '=' sign, it is a KVP with empty key/value and must be preserved.
do {
var url = WebURL("http://example.com?=&=&=")!
XCTAssertEqual(url.serialized(), "http://example.com/?=&=&=")
XCTAssertEqual(url.query, "=&=&=")
XCTAssertEqual(url.formParams.get(""), "")
XCTAssertEqual(url.formParams.getAll("").count, 3)

// Form-encode, ensure KVPs with empty keys/values are preserved.
url.formParams = url.formParams
XCTAssertEqual(url.serialized(), "http://example.com/?=&=&=")
XCTAssertEqual(url.query, "=&=&=")
XCTAssertEqual(url.formParams.get(""), "")
XCTAssertEqual(url.formParams.getAll("").count, 3)
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}
}

func testAppend() {
Expand All @@ -178,16 +197,18 @@ final class FormEncodedQueryParametersTests: XCTestCase {
url.formParams.append("spa ce", value: "") // key needs escaping due to substitution only.
url.formParams.append("search query", value: "why are 🦆 so awesome?") // both need escaping.
url.formParams.append("`back`'tick'", value: "") // U+0027 is encoded by forms, and only by forms.
url.formParams.append("", value: "") // Empty keys and values should be preserved.
XCTAssertEqual(
url.serialized(),
"http://example.com/?non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27="
"http://example.com/?non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27=&="
)
XCTAssertEqual(
url.query, "non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27="
url.query, "non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27=&="
)
XCTAssertEqual(url.formParams.get("non_escaped"), "true")
XCTAssertEqual(url.formParams.get("search query"), "why are 🦆 so awesome?")
XCTAssertEqual(url.formParams.get("`back`'tick'"), "")
XCTAssertEqual(url.formParams.get(""), "")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)

Expand All @@ -209,7 +230,7 @@ final class FormEncodedQueryParametersTests: XCTestCase {
url.formParams = storedParams
XCTAssertEqual(
url.serialized(),
"http://foobar.org/?non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27=&still+alive%3F=should+be%21&owned+and+mutable%3F=sure+thing%21"
"http://foobar.org/?non_escaped=true&spa+ce=&search+query=why+are+%F0%9F%A6%86+so+awesome%3F&%60back%60%27tick%27=&=&still+alive%3F=should+be%21&owned+and+mutable%3F=sure+thing%21"
)
XCTAssertEqual(url.formParams.get("still alive?"), "should be!")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
Expand Down Expand Up @@ -299,6 +320,53 @@ final class FormEncodedQueryParametersTests: XCTestCase {
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}

// Appending an empty collection of KVPs should be a no-op.
do {
var url = WebURL("http://example.com")!
XCTAssertEqual(url.serialized(), "http://example.com/")
XCTAssertNil(url.query)
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)

url.formParams += [] as [(String, String)]
XCTAssertEqual(url.serialized(), "http://example.com/")
XCTAssertNil(url.query)
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}
do {
var url = WebURL("http://example.com?")!
XCTAssertEqual(url.serialized(), "http://example.com/?")
XCTAssertEqual(url.query, "")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)

url.formParams += [] as [(String, String)]
XCTAssertEqual(url.serialized(), "http://example.com/?")
XCTAssertEqual(url.query, "")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}

// Appending a non-empty array of key-value pairs, however the keys and values are empty (!).
// The empty string is a perfectly valid key name/value.
do {
var url = WebURL("http://example.com")!
XCTAssertEqual(url.serialized(), "http://example.com/")
XCTAssertNil(url.query)
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)

url.formParams += [("", "")] as [(String, String)]
XCTAssertEqual(url.serialized(), "http://example.com/?=")
XCTAssertEqual(url.query, "=")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)

url.formParams += [("", ""), ("", ""), ("", "")] as [(String, String)]
XCTAssertEqual(url.serialized(), "http://example.com/?=&=&=&=")
XCTAssertEqual(url.query, "=&=&=&=")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}
}

func testRemove() {
Expand Down Expand Up @@ -340,6 +408,7 @@ final class FormEncodedQueryParametersTests: XCTestCase {
XCTAssertEqual(url.formParams.get("c is the key"), "d")
XCTAssertEqual(url.formParams.get(""), "foo")
url.formParams.remove("c is the key")
XCTAssertEqual(url.serialized(), "http://example.com/?=foo")
url.formParams.remove("")
XCTAssertEqual(url.serialized(), "http://example.com/")
XCTAssertNil(url.query)
Expand Down Expand Up @@ -418,6 +487,26 @@ final class FormEncodedQueryParametersTests: XCTestCase {
XCTAssertEqual(url.query, "c+is+the+key=d&e=collapsed&=foo&h=ALSO+THIS+ONE&doesNotExist=Yes%2C+it+does%21")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)

// It is possible to set a value for the empty key.
url.formParams.set("", to: "hi")
XCTAssertEqual(
url.serialized(),
"http://example.com/?c+is+the+key=d&e=collapsed&=hi&h=ALSO+THIS+ONE&doesNotExist=Yes%2C+it+does%21")
XCTAssertEqual(url.query, "c+is+the+key=d&e=collapsed&=hi&h=ALSO+THIS+ONE&doesNotExist=Yes%2C+it+does%21")
XCTAssertEqual(url.formParams.get(""), "hi")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)

// It is also possible to set an empty value - including for the empty key.
url.formParams.set("", to: "")
XCTAssertEqual(
url.serialized(),
"http://example.com/?c+is+the+key=d&e=collapsed&=&h=ALSO+THIS+ONE&doesNotExist=Yes%2C+it+does%21")
XCTAssertEqual(url.query, "c+is+the+key=d&e=collapsed&=&h=ALSO+THIS+ONE&doesNotExist=Yes%2C+it+does%21")
XCTAssertEqual(url.formParams.get(""), "")
XCTAssertTrue(url.storage.structure.queryIsKnownFormEncoded)
XCTAssertURLIsIdempotent(url)
}

func testInseredValuesAssumedNonEncoded() {
Expand Down

0 comments on commit a675c03

Please sign in to comment.