fix: percent-encode app-controlled path segments - #1704
Conversation
Canonical sort applied by the xcodeproj tool, so later commits in this stack show only their own file additions. No project changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Values the app chooses reach a URL path, where a slash, ?, # or % would name a different endpoint than intended. OSUrlPath centralizes that encoding so every path builder escapes the same way, and is @objc so the in-app messaging path can share it. Live Activities encoded the activity type in the manager and again in the request, so a type holding a reserved character went out double-encoded. Encoding now happens once, where the path is built; the manager keeps its check so the app still hears about a type that cannot be encoded. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model adversarial review (interrogate)
Verdict: request changes
Intent
Percent-encode app-controlled Live Activities URL path segments once via shared
OSUrlPath.segment(CharacterSet.urlUserAllowed), fixing manager+request double-encoding; keep manager encodeability checks; normalizeproject.pbxprojfor the JWT stack. JWT/identity path work is out of scope.
Reviewers
- A:
claude-fable-5-thinking-xhigh— 6 findings - B:
gpt-5.6-sol-xhigh— 2 findings - C:
cursor-grok-4.5-high-fast— 5 findings - D:
claude-opus-5-thinking-high(fallback from unavailable…-xhigh) — 7 findings
Act On
- Start-token cache key identity changes with no migration (4/4) — Pre-PR, manager shadowed
activityTypewith the percent-encoded value and persisted that asOSRequestSetStartToken.keyinStartRequestCache(1-year TTL). This PR stores the raw string. On upgrade, for any type where encoding ≠ identity (spaces,/,%, … — exactly this PR’s population), stale encoded keys remain,remove/appendmiss them, andonPushSubscriptionDidChange→markAllUnsuccessfulcan re-send already-encoded keys throughOSUrlPath.segmentagain (My%20Type→My%2520Type). Fix: decode-on-read / re-key, or bumpOS_LIVE_ACTIVITIES_EXECUTOR_START_TOKENS_KEYto abandon legacy entries (typedAttributes.selfnames are mostly unaffected). - No regression coverage for single-encode (4/4) — Existing Live Activities tests use URL-safe keys and don’t assert
path/URL. A tinyOSUrlPath+ oneprepareForExecutioncase with/or a space would pin the bug this PR exists to fix; deferring all tests to the JWT PR leaves this change unverified.
Consider
- Reject
""/"."/".."inOSUrlPath.segmentso the Optional/throwspath is real and path-shape can’t collapse via empty/dot segments (2–3/4)..inside typed names must still pass. - Prefer a segment-joining path builder (or encode at
OneSignalRequestURL assembly) so “encode once” isn’t six copy-pasted guards (2/4). - Narrow
OSUrlPathdocs: they mentionexternal_id/alias labels and ObjC sharing, but only Live Activities Swift callers are migrated; ObjCreport_receivedstill interpolates raw.
Noted
- Update-token guards still log “activity type” while encoding activity id (pre-existing; line touched).
@objc NSObjectwrapper is forward-looking until an ObjC consumer lands.
Dismissed
- “Block solely because
addingPercentEncodingalmost never returns nil for SwiftString” as a standalone merge blocker — true, but the retainedthrowsis mostly API compatibility; pair with empty/dot rejection or clarify the comment rather than delete the surface in this PR. - Rewriting the whole request pipeline / encoding
appIdin this PR1 — out of stated scope.
Agreement map
Strong consensus on cache-key upgrade hazard and missing single-encode tests. Split on how hard to push empty/dot-segment rejection vs. structural path builder. No model disputed that the double-encode fix for new requests is directionally correct.
Inline comments call out the highest-signal spots in the diff.
Sent by Cursor Automation: Untitled
| guard OSUrlPath.segment(activityType) != nil else { | ||
| throw LiveActivitiesError.invalidActivityType("Cannot translate activity type to url encoded string.") | ||
| } | ||
|
|
There was a problem hiding this comment.
Act on (4/4 reviewers): cache key identity change
Pre-change, this method shadowed activityType with the percent-encoded string and passed that as OSRequestSetStartToken(key:). That key is the dictionary identity in StartRequestCache (persisted, 1-year TTL).
Passing the raw value here fixes double-encoding for new entries, but leaves already-persisted encoded keys orphaned. On upgrade + subscription-id change, markAllUnsuccessful can re-execute a stale key like My%20Type through OSUrlPath.segment → My%2520Type, resurrecting the bug this PR removes. removePushToStartToken with the raw type also cannot supersede the stale entry.
Please migrate/decode-on-read or bump OS_LIVE_ACTIVITIES_EXECUTOR_START_TOKENS_KEY before landing.
| @objc(segment:) | ||
| public static func segment(_ value: String) -> String? { | ||
| return value.addingPercentEncoding(withAllowedCharacters: .urlUserAllowed) | ||
| } |
There was a problem hiding this comment.
Act on / consider (multi-model)
- Tests (4/4): This is a pure helper in a module that already has
OneSignalOSCoreTests, yet nothing asserts/,?,#,%encode once, or that already-encoded input double-encodes as documented. The JWT-PR deferral shouldn’t leave this PR’s core behavior unpinned. - Optional is effectively vacuous for Swift
String:addingPercentEncodingessentially never returnsnilhere, so managerthrows/ request guards never fire for real app inputs (including""). - Path-shape gaps:
urlUserAllowedleaves.alone and accepts"", so".."/ empty can still reshape.../start/<segment>/subscriptions/...after URL normalization. Considerguard !value.isEmpty, value != ".", value != ".."(without encoding every.inside typed type names). - Doc scope: Comment mentions
external_id/ alias labels / ObjC builders, but only Live Activities Swift call sites are migrated in this tip.
| guard let activityType = OSUrlPath.segment(self.key) else { | ||
| OneSignalLog.onesignalLog(.LL_DEBUG, message: "Cannot translate activity type to url encoded string.") | ||
| return false | ||
| } |
There was a problem hiding this comment.
Act on: missing single-encode regression coverage
This is the site that used to second-encode a manager-pre-encoded key. A focused test that builds with key: "a b/c" (or similar), calls prepareForExecution(), and asserts the path contains a single encode (a%20b%2Fc, not %2520 / %252F) would lock the intent. Existing executor tests only use hyphenated safe keys, so they cannot distinguish old vs new behavior.
|
|
||
| guard let activityId = self.key.addingPercentEncoding(withAllowedCharacters: NSCharacterSet.urlUserAllowed) else { | ||
| guard let activityId = OSUrlPath.segment(self.key) else { | ||
| OneSignalLog.onesignalLog(.LL_DEBUG, message: "Cannot translate activity type to url encoded string.") |
There was a problem hiding this comment.
Noted: self.key is an activity id, but the failure log still says “activity type”. Pre-existing copy-paste; this PR rewrote the guard line — cheap to fix while touching it (same on OSRequestRemoveUpdateToken).
|
Closing: Live Activities path-encoding isn't worth a standalone PR (typed Attributes.self names encode to themselves; this was getting ahead of JWT). Keeping only the project.pbxproj normalization as a separate chore. |
1 participant
Description
One Line Summary
Percent-encode app-controlled Live Activities path segments once, via a shared
OSUrlPathhelper, and (chore) normalizeproject.pbxprojordering.Details
Motivation
Values the app chooses can reach a URL path. A slash,
?,#, or%in a Live Activity type would name a different endpoint than intended. The manager was also encoding and the request encoding again, so those types went out double-encoded.The
project.pbxprojnormalization is a one-time canonical UUID sort so later stacked PRs show only their own file additions. There are no logic changes in that file.Scope
OSUrlPathin OneSignalOSCore (also usable later from ObjC)project.pbxprojreorder + membership forOSUrlPath.swiftTesting
Unit testing
No new unit tests in this PR. Path-encoding coverage for user requests lands with the later JWT request-pipeline PR, which is the first consumer of
external_idin paths.Manual testing
Built
UnitTestAppfor iOS Simulator locally; test build succeeded.Affected code checklist
Checklist
Overview
Testing
Final pass