Skip to content

Hoist a shared query-value encoder — six call sites still use raw .urlQueryAllowed for user-typed values #95

Closed
#111
Closed
Hoist a shared query-value encoder — six call sites still use raw .urlQueryAllowed for user-typed values#95
#111
Labels
tech-debtCleanup / dead code / correctness hygienews:papercutsW8 — small parity papercuts

Description

@Adron
Adron
opened on Sep 16, 2026

Surfaced while fixing #77 (see PR #93).

The hazard

CharacterSet.urlQueryAllowed permits +, &, =, ?, # and /. Percent-encoding a query
value with it therefore leaves those characters intact, and:

  • a raw + decodes to a space on the server, and
  • a raw & or = splits the value into another query parameter.

For a search box, that means a term like a & b or c++ silently searches for something else, or
truncates. For an opaque cursor it means paging quietly breaks — the page repeats or ends early,
with no error anywhere.

Call sites still using the raw set

All passing user-typed or server-issued opaque values (Services/APIClient.swift, approx. lines):

~Line What it encodes
640 search term
709 search term
1097 search term
1209 search term
1325 search term
1467 pagination cursor

The cursor one is the most likely to be actively broken today, since base64 routinely contains +
and =.

Three copies of the fix already exist

Each was written locally because the branches were independent:

Three occurrences is past the point where it should be one function.

Build this

  • Hoist a single queryValue(_:) into APIClientTransport.swift alongside the other transport
    helpers (internal, not private — private is file-scoped and the per-feature extension
    files could not see it).
    Allowed set: .urlQueryAllowed minus CharacterSet(charactersIn: "+&=?#/").
  • Replace all three local copies with it.
  • Convert the six raw .urlQueryAllowed call sites above.
  • Tests: a search term containing +, &, = and a space round-trips unchanged through
    URLComponents; a base64 cursor containing +, / and = round-trips unchanged.

Sequencing

Touches APIClient.swift and both extension files, so it will conflict with anything else in flight
there. Best run after #86 and #93 merge, so all three copies exist in one tree and can be
collapsed in a single pass.

Acceptance criteria

  • Exactly one query-value encoder in the codebase.
  • No remaining addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) on a query
    value (path encoding via .urlPathAllowed is a different thing and stays).
  • Suite green; no behaviour change other than correct encoding.

Files: Services/APIClientTransport.swift, Services/APIClient.swift,
Services/APIClient+Organizations.swift, Services/APIClient+DirectMessages.swift.

Activity

  1. Adron
    added
    tech-debtCleanup / dead code / correctness hygiene
    ws:papercutsW8 — small parity papercuts
    on Sep 16, 2026
  2. Adron
    closed this as completedin #111on Sep 17, 2026

  3. Adron commented on Sep 17, 2026

    @Adron
    Adron
    on Sep 17, 2026
    MemberAuthor

    Implemented in #111, now merged. Closing.

    The test this issue specified would not have caught the main hazard

    This issue asked for "a search term containing +, &, = and a space round-trips unchanged
    through URLComponents". That assertion passes against the broken encoder, which I verified
    directly:

    broken wire form: c++%20tips
fixed  wire form: c%2B%2B%20tips
broken: round-trip -> "c++ tips"  equals original? true    <-- would have passed
fixed:  round-trip -> "c++ tips"  equals original? true

    URLComponents preserves a literal + when decoding, so only & and = make the round-trip fail.
    But + is the one actively breaking base64 cursors, because the backend reads params through
    URLSearchParams, which decodes + as a space.

    #111 asserts on the wire form instead (XCTAssertFalse(rawQuery.contains("+"))), which is the
    check that actually bites. Worth remembering for the next encoding test: assert what goes on the
    wire, not what a client-side parser hands back.

    Outcome

    • Exactly the three copies this issue predicted, and exactly six raw call sites — five search terms
      plus the dmThreadUpdates after cursor. Line numbers had drifted; the set had not.
    • Three already-correct sites were repointed at the shared helper for free.
    • One shared queryValue(_:) now lives in APIClientTransport.swift beside pathSegment(_:), with
      a comment saying why they are not interchangeable.
    • Tests mutation-checked: reverting the allowed set produced 32 assertion failures across 13
      tests      , including the three pre-existing ones from feat(messages): group the DM inbox by conversation #86, fix(identities): send unlink provider as a query param #93 and the org search. The path-encoding
      guard correctly stayed green.
    • 1330 tests, 0 failures.

    One behaviour delta, harmless

    The old orgQueryValueAllowed stripped &=+?# but not /; the shared set also strips /. Org
    search values now go over the wire with %2F rather than a bare slash — strictly more encoding,
    decoding identically server-side. excludeMembers commas are untouched, since , is not in the
    strip set, and the test pinning "the backend splits this list on a literal comma" still passes.

    Noted, not fixed

    About a dozen path segments are still encoded inline as
    id.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? id even though
    pathSegment(_:) exists for exactly that. Correct as written, just not using the helper — same
    rule-of-three argument, but a separate sweep. Filed as #112.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    tech-debtCleanup / dead code / correctness hygienews:papercutsW8 — small parity papercuts

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions