From f1433e83d814c74d17ec8b88056d567299070777 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Thu, 17 Sep 2026 02:40:08 -0700 Subject: [PATCH] refactor(api): collapse three query-value encoders into one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.urlQueryAllowed` permits `+ & = ? # /`, so percent-encoding a query *value* with it leaves them intact: a raw `+` reaches the backend as a space (params are read via `URLSearchParams`) and a raw `&` or `=` splits the value into another query parameter. Six call sites still passed user-typed search terms and an opaque cursor through the raw set, while three separate local fixes for the same hazard had accumulated in parallel branches. Hoist one `queryValue(_:)` into `APIClientTransport.swift`, next to `pathSegment(_:)` under a shared "URL encoding" heading so the next caller sees both and picks deliberately. It is `internal`, not `private`, so the per-feature `APIClient+*.swift` extensions can reach it. The three local copies (`orgQueryValueAllowed`, `encodedQueryValue`, `queryValueAllowed`) are gone. Path encoding via `.urlPathAllowed` is untouched — different job, different allowed set. Closes #95. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017bss5MgZa7Jvj2m9zdaUd1 --- InterlinedList.xcodeproj/project.pbxproj | 4 + .../Services/APIClient+DirectMessages.swift | 11 +- .../Services/APIClient+Organizations.swift | 18 +-- InterlinedList/Services/APIClient.swift | 25 +--- .../Services/APIClientTransport.swift | 22 +++- .../APIClientQueryValueEncodingTests.swift | 121 ++++++++++++++++++ 6 files changed, 155 insertions(+), 46 deletions(-) create mode 100644 InterlinedListTests/APIClientTests/APIClientQueryValueEncodingTests.swift diff --git a/InterlinedList.xcodeproj/project.pbxproj b/InterlinedList.xcodeproj/project.pbxproj index 39f5a43..80fa951 100644 --- a/InterlinedList.xcodeproj/project.pbxproj +++ b/InterlinedList.xcodeproj/project.pbxproj @@ -65,6 +65,7 @@ 5F81C584710958DCBF85F60E /* AIServiceError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0AC10FAF5FB93F8317D2D91A /* AIServiceError.swift */; }; 61621697746E8CF1769E9C80 /* IdentityHealth.swift in Sources */ = {isa = PBXBuildFile; fileRef = 80D1D19E4DB2D914AD6551D7 /* IdentityHealth.swift */; }; 64F5804ECC25725FD1E58E84 /* APIClientModerationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6A569D8DB8DFD3CC59072FDB /* APIClientModerationTests.swift */; }; + 9F0095A1C4E24B7D0095E002 /* APIClientQueryValueEncodingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9F0095A1C4E24B7D0095E001 /* APIClientQueryValueEncodingTests.swift */; }; C0FFEE9102ABCDEF00000091 /* ComposeLinkDetectionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0FFEE9101ABCDEF00000091 /* ComposeLinkDetectionTests.swift */; }; 6749119D27FA93BE00D5A27F /* FeedTruncationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 05A695D62B746B092CF51AFA /* FeedTruncationTests.swift */; }; 6A89622E299B0D172D5B5556 /* GitHubModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C337406C10876F331B1888E3 /* GitHubModelTests.swift */; }; @@ -274,6 +275,7 @@ 0221B233F89B966D044C37FA /* ServerLimits.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = ServerLimits.swift; sourceTree = ""; }; 03C47A3ECFD7F4D25F91E3BE /* ShareLinksSheet.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = ShareLinksSheet.swift; sourceTree = ""; }; 03D6C33FD59101503195F14F /* SharedDocumentView.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SharedDocumentView.swift; sourceTree = ""; }; + 9F0095A1C4E24B7D0095E001 /* APIClientQueryValueEncodingTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = APIClientQueryValueEncodingTests.swift; sourceTree = ""; }; C0FFEE9101ABCDEF00000091 /* ComposeLinkDetectionTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = ComposeLinkDetectionTests.swift; sourceTree = ""; }; 05A695D62B746B092CF51AFA /* FeedTruncationTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = FeedTruncationTests.swift; sourceTree = ""; }; 06CAA8B0033D6DD27ED089CD /* ShareInvitesSheet.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = ShareInvitesSheet.swift; sourceTree = ""; }; @@ -824,6 +826,7 @@ E46B33ABF62A3438ADF98885 /* APIClientAppSettingsTests.swift */, FCCD5BA8427B40000116FC43 /* APIClientDocumentPresenceTests.swift */, EA635B97637F46345E2F6347 /* APIClientDMConversationsTests.swift */, + 9F0095A1C4E24B7D0095E001 /* APIClientQueryValueEncodingTests.swift */, ); path = APIClientTests; sourceTree = ""; @@ -1238,6 +1241,7 @@ 572EF654CD322007FCC0A68C /* DocumentPresenceServiceTests.swift in Sources */, 9DFDF38F0AFBF42EB781E5E5 /* ListRowSortingTests.swift in Sources */, D74FA886D6F1AE873E679EA5 /* APIClientDMConversationsTests.swift in Sources */, + 9F0095A1C4E24B7D0095E002 /* APIClientQueryValueEncodingTests.swift in Sources */, F49DE4618E3EDFBDE2D66B1A /* ListRowComposeTextTests.swift in Sources */, F5ADADB7D863EAB9BD4996C6 /* ViewPreferencesTests.swift in Sources */, 9C983C24B85DBE20FCDF1873 /* APIClientAppSettingsTests.swift in Sources */, diff --git a/InterlinedList/Services/APIClient+DirectMessages.swift b/InterlinedList/Services/APIClient+DirectMessages.swift index 48bc4e4..3f6d3a5 100644 --- a/InterlinedList/Services/APIClient+DirectMessages.swift +++ b/InterlinedList/Services/APIClient+DirectMessages.swift @@ -15,7 +15,7 @@ extension APIClient { func dmConversations(cursor: String? = nil, take: Int? = nil) async throws -> DMConversationPage { var query: [String] = [] if let cursor, !cursor.isEmpty { - query.append("cursor=" + Self.encodedQueryValue(cursor)) + query.append("cursor=" + queryValue(cursor)) } if let take { query.append("take=\(take)") @@ -23,13 +23,4 @@ extension APIClient { let suffix = query.isEmpty ? "" : "?" + query.joined(separator: "&") return try await get("/api/dm/conversations" + suffix) } - - /// `.urlQueryAllowed` permits `+`, `=`, `&` and `/`, all of which appear in the - /// base64 keyset cursor this route issues. Left unescaped, a `+` decodes as a - /// space server-side and the cursor silently stops matching — the page repeats - /// or ends early. Escape them explicitly. - static func encodedQueryValue(_ raw: String) -> String { - let allowed = CharacterSet.urlQueryAllowed.subtracting(CharacterSet(charactersIn: "+&=?#/")) - return raw.addingPercentEncoding(withAllowedCharacters: allowed) ?? raw - } } diff --git a/InterlinedList/Services/APIClient+Organizations.swift b/InterlinedList/Services/APIClient+Organizations.swift index d8b5700..97fef5f 100644 --- a/InterlinedList/Services/APIClient+Organizations.swift +++ b/InterlinedList/Services/APIClient+Organizations.swift @@ -5,18 +5,6 @@ import Foundation -/// `.urlQueryAllowed` deliberately permits the sub-delimiters `&`, `=`, `+` and -/// `?` — they are legal *somewhere* in a query string. Encoding a value with it -/// therefore lets a user-typed `&` split the query into an extra parameter, so a -/// search for `"ada l&ve"` reaches the backend as `search=ada l`. Strip the -/// delimiters so a value stays one value. `+` is included because the backend -/// reads params via `URLSearchParams`, which decodes `+` as a space. -private let orgQueryValueAllowed: CharacterSet = { - var allowed = CharacterSet.urlQueryAllowed - allowed.remove(charactersIn: "&=+?#") - return allowed -}() - /// Organization discovery and member recruitment. /// /// These are the two reads that `APIClient`'s existing `addOrganizationMember` @@ -43,13 +31,11 @@ extension APIClient { let encoded = id.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? id var path = "/api/organizations/\(encoded)/users?limit=\(limit)&offset=\(offset)" if let search, !search.isEmpty { - let query = search.addingPercentEncoding(withAllowedCharacters: orgQueryValueAllowed) ?? search - path += "&search=\(query)" + path += "&search=\(queryValue(search))" } if let excludeMembers, !excludeMembers.isEmpty { let joined = excludeMembers.joined(separator: ",") - let query = joined.addingPercentEncoding(withAllowedCharacters: orgQueryValueAllowed) ?? joined - path += "&excludeMembers=\(query)" + path += "&excludeMembers=\(queryValue(joined))" } let response: OrganizationUsersResponse = try await get(path) return response.users diff --git a/InterlinedList/Services/APIClient.swift b/InterlinedList/Services/APIClient.swift index 62f8671..1da0884 100644 --- a/InterlinedList/Services/APIClient.swift +++ b/InterlinedList/Services/APIClient.swift @@ -176,20 +176,10 @@ final class APIClient { /// with two rows for the same provider loses both); the identity's own id is /// not read at all. func unlinkIdentity(provider: String) async throws { - let encoded = provider.addingPercentEncoding(withAllowedCharacters: Self.queryValueAllowed) ?? provider + let encoded = queryValue(provider) try await delete("/api/user/identities?provider=\(encoded)") } - /// `.urlQueryAllowed` permits the sub-delimiters `+ & = ? # /` — legal - /// *somewhere* in a query string, but inside a single value they end it or - /// split it into another parameter. `+` matters most: the backend reads params - /// through `URLSearchParams`, which decodes a literal `+` as a space. - private static let queryValueAllowed: CharacterSet = { - var allowed = CharacterSet.urlQueryAllowed - allowed.remove(charactersIn: "+&=?#/") - return allowed - }() - /// Carries the verify route's two "credential is unusable" statuses out of the /// transport, which can only signal by throwing, back to the outcome value /// `verifyIdentity` returns. @@ -703,7 +693,7 @@ final class APIClient { } func searchDocuments(q: String, limit: Int = 20, offset: Int = 0) async throws -> ([Document], Pagination?) { - let qEncoded = q.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? q + let qEncoded = queryValue(q) struct Response: Decodable { let documents: [Document]; let pagination: Pagination? } let response: Response = try await get("/api/documents/search?q=\(qEncoded)&limit=\(limit)&offset=\(offset)") return (response.documents, response.pagination) @@ -775,7 +765,7 @@ final class APIClient { } func searchLists(q: String, limit: Int = 20, offset: Int = 0) async throws -> ([UserList], Pagination?) { - let qEncoded = q.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? q + let qEncoded = queryValue(q) struct Response: Decodable { let lists: [UserList]; let pagination: Pagination? } let response: Response = try await get("/api/lists/search?q=\(qEncoded)&limit=\(limit)&offset=\(offset)") return (response.lists, response.pagination) @@ -1176,8 +1166,7 @@ final class APIClient { let encoded = listId.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? listId var path = "/api/lists/\(encoded)/watchers/users?limit=\(limit)&offset=\(offset)" if let search, !search.isEmpty { - let q = search.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? search - path += "&search=\(q)" + path += "&search=\(queryValue(search))" } let response: WatcherCandidatesResponse = try await get(path) return response.users @@ -1288,7 +1277,7 @@ final class APIClient { func searchDocumentCollaboratorCandidates(id: String, query: String) async throws -> [WatcherCandidate] { let encoded = id.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? id - let q = query.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? query + let q = queryValue(query) let response: WatcherCandidatesResponse = try await get("/api/documents/\(encoded)/collaborators/users?q=\(q)") return response.users } @@ -1399,7 +1388,7 @@ final class APIClient { // MARK: - Message search (Phase 13 / B2) func searchMessages(q: String, limit: Int = 20, offset: Int = 0) async throws -> (messages: [Message], pagination: Pagination?) { - let qEncoded = q.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? q + let qEncoded = queryValue(q) let response: MessagesResponse = try await get("/api/messages/search?q=\(qEncoded)&limit=\(limit)&offset=\(offset)") return (response.messages, response.pagination) } @@ -1527,7 +1516,7 @@ final class APIClient { /// Auto-marks received messages read. func dmThreadUpdates(username: String, after: String) async throws -> DMThread { let encodedUser = username.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? username - let encodedAfter = after.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? after + let encodedAfter = queryValue(after) return try await get("/api/dm/thread/\(encodedUser)/updates?after=\(encodedAfter)") } diff --git a/InterlinedList/Services/APIClientTransport.swift b/InterlinedList/Services/APIClientTransport.swift index 637afb7..18eb667 100644 --- a/InterlinedList/Services/APIClientTransport.swift +++ b/InterlinedList/Services/APIClientTransport.swift @@ -8,6 +8,12 @@ import os.log private let transportLog = Logger(subsystem: "com.interlinedlist.app", category: "APIClient") +/// `.urlQueryAllowed` permits the sub-delimiters `+ & = ? # /` — legal *somewhere* +/// in a query string, but inside a single value they end it or split it into +/// another parameter. `+` matters most: the backend reads params through +/// `URLSearchParams`, which decodes a literal `+` as a space. +private let queryValueAllowed = CharacterSet.urlQueryAllowed.subtracting(CharacterSet(charactersIn: "+&=?#/")) + /// The HTTP seam every `APIClient` endpoint is built on: URL assembly, auth /// header, body encoding, status checking, decoding. /// @@ -192,12 +198,24 @@ extension APIClient { return data } - /// Percent-encodes one path segment. `?? segment` keeps the call sites free - /// of force-unwraps; encoding only fails for inputs a path can't hold anyway. + // MARK: - URL encoding + + /// Percent-encodes one path *segment* — the part between two slashes. `?? + /// segment` keeps the call sites free of force-unwraps; encoding only fails + /// for inputs a path can't hold anyway. func pathSegment(_ segment: String) -> String { segment.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? segment } + /// Percent-encodes one query *value* — whatever follows a `=` in the query + /// string, typically a user-typed search term or a server-issued opaque + /// cursor. Not interchangeable with `pathSegment(_:)`: see + /// `queryValueAllowed` for the sub-delimiters this has to strip and that one + /// must not. + func queryValue(_ value: String) -> String { + value.addingPercentEncoding(withAllowedCharacters: queryValueAllowed) ?? value + } + // MARK: - Private private func jsonRequest(_ path: String, method: String, authenticated: Bool = true) throws -> URLRequest { diff --git a/InterlinedListTests/APIClientTests/APIClientQueryValueEncodingTests.swift b/InterlinedListTests/APIClientTests/APIClientQueryValueEncodingTests.swift new file mode 100644 index 0000000..75e16ed --- /dev/null +++ b/InterlinedListTests/APIClientTests/APIClientQueryValueEncodingTests.swift @@ -0,0 +1,121 @@ +import XCTest +@testable import InterlinedList + +/// Every route that puts a user-typed term or a server-issued opaque cursor into +/// a query value, checked through the one shared `queryValue(_:)` encoder. +/// +/// Two failure modes are being guarded against, and only one of them is visible +/// to `URLComponents`: a raw `&` or `=` splits the value into an extra query +/// parameter (so the round-trip and the parameter count both catch it), while a +/// raw `+` survives `URLComponents` intact and only goes wrong on the backend, +/// which reads params through `URLSearchParams` and decodes `+` as a space. +/// That is why each case also asserts the *wire* form carries no literal `+`. +final class APIClientQueryValueEncodingTests: XCTestCase { + var sut: APIClient! + var session: MockURLSession! + + /// Contains all four of `+`, `&`, `=` and a space. + private let awkwardTerm = "c++ & a=b" + /// Base64 alphabet output, containing `+`, `/` and the `=` padding. + private let base64Cursor = "YWJjKz0vZGVm+/w==" + + override func setUp() { + super.setUp() + session = MockURLSession() + sut = APIClient(session: session) + sut.setBearerToken("tok") + } + + private func assertQueryValueSurvives(_ expected: String, + named name: String, + parameterCount: Int, + file: StaticString = #filePath, + line: UInt = #line) throws { + let url = try XCTUnwrap(session.lastRequest?.url, file: file, line: line) + let rawQuery = url.query ?? "" + let items = URLComponents(url: url, resolvingAgainstBaseURL: false)?.queryItems ?? [] + XCTAssertEqual(items.count, parameterCount, + "The value must not split into extra parameters: \(rawQuery)", + file: file, line: line) + XCTAssertEqual(items.first(where: { $0.name == name })?.value, expected, + "Expected \(name) to round-trip unchanged: \(rawQuery)", + file: file, line: line) + XCTAssertFalse(rawQuery.contains("+"), + "A literal + reaches the backend as a space: \(rawQuery)", + file: file, line: line) + } + + // MARK: - Search terms + + func test_searchMessages_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"messages":[],"pagination":null}"#) + _ = try await sut.searchMessages(q: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "q", parameterCount: 3) + } + + func test_searchDocuments_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"documents":[],"pagination":null}"#) + _ = try await sut.searchDocuments(q: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "q", parameterCount: 3) + } + + func test_searchLists_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"lists":[],"pagination":null}"#) + _ = try await sut.searchLists(q: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "q", parameterCount: 3) + } + + func test_searchWatcherCandidates_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"users":[],"total":0}"#) + _ = try await sut.searchWatcherCandidates(listId: "list-1", search: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "search", parameterCount: 3) + } + + func test_searchDocumentCollaboratorCandidates_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"users":[],"total":0}"#) + _ = try await sut.searchDocumentCollaboratorCandidates(id: "doc-1", query: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "q", parameterCount: 1) + } + + func test_organizationUsers_termWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(json: #"{"users":[],"total":0}"#) + _ = try await sut.organizationUsers(id: "org-1", search: awkwardTerm) + try assertQueryValueSurvives(awkwardTerm, named: "search", parameterCount: 3) + } + + // MARK: - Opaque cursors + + func test_dmThreadUpdates_base64CursorWithPadding_roundTripsUnchanged() async throws { + session.stub(json: threadJSON) + _ = try await sut.dmThreadUpdates(username: "bob", after: base64Cursor) + try assertQueryValueSurvives(base64Cursor, named: "after", parameterCount: 1) + } + + func test_dmConversations_base64CursorWithPadding_roundTripsUnchanged() async throws { + session.stub(json: #"{"items":[],"nextCursor":null}"#) + _ = try await sut.dmConversations(cursor: base64Cursor) + try assertQueryValueSurvives(base64Cursor, named: "cursor", parameterCount: 1) + } + + func test_unlinkIdentity_providerWithQueryDelimiters_roundTripsUnchanged() async throws { + session.stub(data: Data(), statusCode: 204) + try await sut.unlinkIdentity(provider: "mastodon:a+b&c=d") + try assertQueryValueSurvives("mastodon:a+b&c=d", named: "provider", parameterCount: 1) + } + + // MARK: - Path encoding is left alone + + /// `pathSegment(_:)` and `queryValue(_:)` are not interchangeable: a slash in + /// a *path* segment still has to be escaped, and encoding the two halves of a + /// URL with one set would break whichever half it wasn't chosen for. + func test_dmThreadUpdates_usernameStaysPathEncoded() async throws { + session.stub(json: threadJSON) + _ = try await sut.dmThreadUpdates(username: "bob smith", after: "m1") + XCTAssertEqual(session.lastRequest?.url?.path, "/api/dm/thread/bob smith/updates") + } + + private let threadJSON = #""" + {"items":[],"olderCursor":null,"isMutual":true,"isBlocked":false, + "otherUser":{"id":"r1","username":"bob","displayName":"Bob","avatar":null}} + """# +}