feat: add dev mode for previewing local paywalls - #511
Conversation
|
PR author is not in the allowed authors list. |
…rk-support # Conflicts: # SuperwallKit.xcodeproj/project.pbxproj
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dev mode is a new feature, so the staged release gets a minor bump instead of a patch. Bumps the version in all three places. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Two things reach production builds that shouldn't: DevServerPreview.handle reports a deep link as "handled by Superwall" before it checks whether dev mode is on, and the superwall_dev origin is attacker-suppliable while the paywall web view applies no origin check to its JS bridge. There is also a ## Unreleased heading the repo conventions forbid and a SwiftLint trailing-whitespace violation.
Reviewed changes — full initial review of the dev-mode feature: the new DevServer/ subsystem, its wiring into the paywall request pipeline, test mode and config, the debugger's new picker, and the four new test files.
DevModegate —isActive(_:)requiresdevMode/devServerURLANDDeviceHelper.isSandboxEnvironment(new), so an App Store production build ignores the option and logs a one-time warning.DevServerManifest/DevServerLocator— decodesGET {base}/device/manifest.json, resolves a dashboard paywall to a local surface via asuperwall.lockbinding (or the single-paywall fallback), and walkslocalhost:6100…6104unlessdevServerURLis set, with a 2s hit cache and 5s miss debounce.- Paywall override —
applyDevServerOverrideIfNeededrewritesurl/urlConfigand nilsmanifeston every non-debugger paywall response;Paywall's stubinitbecomes internal andurlConfig/manifestbecomevar. superwall_devdeep link — parsed out of any inbound URL, pins the locator's base and opens the debugger on the named surface.- Test mode & config — dev mode force-enables test mode, skips paywall preloading, and replaces the test-mode intro modal with
applyDefaultTestModeState(a faithful mirror of the modal's "no entitlements" branch). - Debugger picker —
pressedPreviewnow presents a searchable, sectioned sheet (DebugPickerLogic+DebugPaywallPickerViewController) listing local surfaces and published paywalls, replacing the old alert. - Tests — pure-function coverage for the picker sections, manifest decoding/resolution, deep-link parsing, the
DevModeproduction gate, and the synthesisedPaywall.
⚠️ Host apps are told to ship an app-wide ATS downgrade for a dev-only feature
The devMode doc comment, the CHANGELOG entry and Examples/Basic/Basic/Info.plist all instruct apps to add NSAllowsArbitraryLoadsInWebContent alongside NSAllowsLocalNetworking. The first key disables App Transport Security for all web content in the app, permanently and in production, and is a documented App Review justification trigger — a steep price for a feature that only ever talks to localhost or a private-range IP.
Technical details
# ATS guidance should be the narrowest key that works, and scoped to debug builds
## Affected sites
- `Sources/SuperwallKit/Config/Options/SuperwallOptions.swift:402-404` — doc comment tells every host app to add both keys
- `CHANGELOG.md` — the shipped release note repeats the same instruction
- `Examples/Basic/Basic/Info.plist:16-22` — the example app models the broad key for customers to copy
- `Sources/SuperwallKit/DevServer/DevServerManifest.swift:130-134` — the runtime error message prints both keys as the fix
## Required outcome
- Confirm empirically whether `NSAllowsLocalNetworking` alone lets a `WKWebView` load `http://localhost:6100` and `http://192.168.x.x:6100`. If it does, drop `NSAllowsArbitraryLoadsInWebContent` from all four places.
- If the broad key really is required for the web view, say so explicitly in the docs and steer developers to a debug-only `Info.plist` (separate build configuration / `INFOPLIST_FILE` per config) rather than their shipping one.
## Open questions for the human
- Is there an existing Superwall docs page for `superwall dev` that carries this guidance? It should match whatever lands here.ℹ️ The parts of dev mode that carry the risk have no test coverage
The four new test files cover pure functions only — manifest decoding, DebugPickerLogic.sections, deep-link parsing, the synthesised Paywall, and the DevMode production gate. The stateful pieces where the bugs flagged inline actually live are untested: DevServerLocator.locate (port walking, pin, hit/miss TTLs, candidate reordering), applyDevServerOverrideIfNeeded, and ConfigManager.applyDefaultTestModeState.
Technical details
# Add coverage for the stateful dev-mode paths
## Affected sites
- `Sources/SuperwallKit/DevServer/DevServerManifest.swift:62-163` — `DevServerLocator` has no test at all
- `Sources/SuperwallKit/Paywall/Request/Operators/RawPaywallResponse.swift:33-66` — `applyDevServerOverrideIfNeeded` has no test
- `Sources/SuperwallKit/Config/ConfigManager.swift:737-749` — `applyDefaultTestModeState` has no test
## Required outcome
- A test that pins `applyDefaultTestModeState` to the same end state as `presentTestModeModal`'s "no entitlements selected" branch, so the two can't silently drift.
- A test over `DevServerLocator`'s candidate ordering that would fail if the `pin`/cached reordering regressed (this is what would have caught the invalid sort comparator flagged inline).
- A test that `applyDevServerOverrideIfNeeded` leaves the paywall untouched when no server is reachable, and rewrites `url`, `urlConfig` and `manifest` when one is.
## Suggested approach (optional)
- `DevServerLocator`'s network hop is the obstacle. Following the `DevMode.isSandboxEnvironment` precedent, an injectable fetch closure would make `locate` testable without a live server.ℹ️ Nitpicks
- All four new test files use XCTest.
CLAUDE.mdstates this project uses Swift's Testing framework (@Test/#expect) for all unit tests and that new tests should always use it. DevModeTestsmutates the globalDevMode.isSandboxEnvironmentand restores it intearDown. Several suites here already flake under parallel execution for exactly this reason — worth knowing if these start failing only in parallel runs.DebugViewController.loadDevServerPreviewcallsDevServerLocator.shared.locate(...)again even thoughdevServerwas already resolved byensureDevServerand carries the same base; onlylocation.manifestis used from the second call.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ Delta since the last review is release staging only. Nothing new to flag; the earlier findings are still open.
Reviewed changes — the three commits added since my review of 6519c3c. No behavioral change in this delta.
- Merge from
develop(4f1779f) — brings in the SDK-configuration data-race fix already on the base branch. - CHANGELOG restaged (
48b6b35,5d8c076) — the## Unreleasedheading is gone; the dev-mode entry now sits under## 4.17.0as### Enhancements, above the fixes that were previously staged as 4.16.4. - Version bump —
Constants.swift(still on line 21),SuperwallKit.podspecandCHANGELOG.mdall read 4.17.0 consistently. project.pbxprojregenerated — wholesale identifier churn fromxcodegen; the newDevServer/group and all six new files are still wired into both targets.
ℹ️ The version restage went further than the convention prescribes
CLAUDE.md says that when develop's version is already above master's — 4.16.4 vs 4.16.3 here — a release is staged and you add entries to that section without bumping again. Instead the staged 4.16.4 was renamed to 4.17.0, which pulls two already-staged fixes into a minor release. That is arguably the better semver call for a PR adding new public API (devMode, devServerURL), so I am flagging it only so whoever owns the release train is aware the number moved rather than assuming a plain patch is still queued. No change requested.
ℹ️ Still open from the previous review
Nothing in this delta touched the earlier findings, so they carry forward unchanged: the ungated superwall_dev deep-link claim in production builds, the attacker-suppliable dev-server origin reaching the paywall JS bridge, the dev-server override being frozen into paywallsByHash, withTimeout not bounding wall-clock time, the invalid sort comparator in DevServerLocator, and the trailing whitespace at Sources/SuperwallKit/Debug/DebugViewController.swift:128 (verified still present at 5d8c076).
Claude Opus | 𝕏
Addresses pullfrog's review of 6519c3c: - handleDeepLink no longer claims a superwall_dev link when dev mode is off, so production apps keep routing such URLs down their handler chain. The pre-configuration storeDeepLink path only claims dev links once options are checkable. - A deep-link-supplied dev-server base must now be a host superwall dev could have printed (loopback, .local, private-network ranges) or match the developer-supplied devServerURL, so an arbitrary internet host can no longer be handed the paywall JS bridge. - Dev-mode paywalls skip the request-hash memoisation and fold the mount URL into cacheKey, so a transient server miss no longer pins the published paywall for the process and a moved server reloads the web view. - The debugger's withTimeout now genuinely resumes at the deadline instead of waiting out the slow product call and discarding it. - The cached-base move-to-front uses removeAll/insert instead of an irreflexive sort predicate. - Removes trailing whitespace flagged by SwiftLint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Important
The new isTrustedBase allowlist constrains the deep-link base but nothing re-applies that boundary to the mount URL the manifest names, so the origin guarantee its own doc comment states is not actually enforced. Everything else in this delta checks out: the withTimeout race, the cache-key/memoisation change and the comparator fix are all correct.
Reviewed changes — the single commit added since my review of 5d8c076 (154f0a2), which addresses all seven inline threads from the initial review.
- Dev links gated out of production —
DevServerPreview.handlenow checksDevMode.isActivesynchronously and returnsfalse(still logging the warning) when dev mode is off, andDeepLinkRouter.isSuperwallURLclaims the link only onceSuperwall.isInitializedandcanHandleboth agree.storeDeepLinkruns both pre-configureand configured-but-not-yet-loaded, so theisInitializedgate is not vacuous. - Deep-link origin allowlisted —
isTrustedBaserestricts a deep-link-supplied base to loopback,::1,*.local, RFC1918/link-local ranges, or an exact scheme+host+port match onoptions.devServerURL. The four-numeric-octet parse correctly rejects10.0.0.1.evil.example.com, and userinfo smuggling, case, trailing-dot FQDNs and decimal/octal IP forms all fail closed. withTimeoutrewritten as a real race — anAsyncStreamwhose first yield wins, so the caller resumes at the deadline instead of waiting out the operation and then discarding its result. No deadlock, hang or double-resume is reachable: the build closure runs synchronously,try?aroundTask.sleepguarantees the timeout branch always yields, andAsyncStream.Continuationis documented idempotent.- Dev-mode cache unfrozen —
saveRequestHashskips thepaywallsByHashmemoisation while dev mode is active (concurrent work still dedupes throughactiveTasks), and the override folds the mount URL intoPaywall.cacheKeyso a moved server or a published fallback takes.replacePaywall/.loadWebViewinstead of.updatePaywall. - Comparator and lint fixes — the non-strict-weak-ordering
sortpredicate inDevServerLocatorbecameremoveAll/insert, matching thepinnedBaseblock above it, and the trailing whitespace atDebugViewController.swift:128is gone. - Tests — a new Swift Testing
DevServerPreviewTestscovering the trusted and untrusted host lists,devServerURLmatching andcanHandle, plus aDeepLinkRouterTestscase that does fail against the pre-fix code.
Claude Opus | 𝕏
A manifest fetched from a trusted base could still name an absolute URL on any origin, since URL(string:relativeTo:) ignores the base for absolute strings. mountURL now rejects any resolved URL whose scheme, host, or port differs from the base, covering both the request-pipeline and debugger callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pullfrog stalled The agent stopped emitting events for 125s and was killed by the activity-timeout watchdog. The model produced no output at all before the stall — the request was sent but nothing came back. This is usually transient; re-running often succeeds.
|
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes — the single commit added since my review of 154f0a2 (75da4e8), which applies the mount-URL origin pin from that review's only open thread.
- Mount URLs pinned to the manifest's origin —
DevServerManifest.mountURL(for:base:)now returnsnilunless the resolved scheme, host and port all equalbase's, so a manifest fetched from a trusted address can no longer name an arbitrary internet origin. One place, covering both the request-pipeline caller (RawPaywallResponse.swift:41) and the debugger (DebugViewController.swift:411). - Regression test added —
test_mountUrlRejectsSurfacesPointingOffTheDevServerOriginassertsnilfor absolute, protocol-relative and other-port surface urls. All three resolve non-nilagainst the pre-fix code, so the case genuinely fails without the guard rather than passing either way. - No over-rejection of legitimate manifests — checked against Foundation's
URLsemantics: a relativesurface.urlinheritsbase's scheme/host/port and still passes (test_mountUrlResolvesAgainstTheDevServerOrigin), as do an absolute same-origin url and a protocol-relative same-host url. - Threat scope is unchanged — worth stating plainly so the fix isn't read as more than it is: the pin stops a manifest from redirecting off-origin, not a hostile server actually resident at a trusted private address, which still reaches the paywall JS bridge over plain HTTP. That was the open question on the previous thread and the author has deferred it to the humans; no change requested here.
Claude Opus | 𝕏
A representation mismatch (localhost vs 127.0.0.1, or a portless devServerURL against an explicit-port surface url) would otherwise disable the override with no trace, which is the one failure mode this subsystem otherwise always logs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit added since my review of 75da4e8 (4a032b8), which applies the only open suggestion from that review. Log-only delta.
- Off-origin rejections are now traceable —
DevServerManifest.mountURL(for:base:)emits a.warnon.superwallCorebefore returningnilfrom the origin-pin guard, naming the surface id, its rawurl, the resolved absolute URL andbase. Both callers still treatnilas a silent no-op, but thenilitself now always carries a log from insidemountURL, which was the ask. - Diagnostic quality is right for the failure it covers — the message distinguishes the raw
surface.urlfrom what it resolved to, so the representation mismatches this guard can hit (localhostvs127.0.0.1, or adevServerURLgiven without a port against an absolute:6100surface url) read straight out of the log rather than looking like "no dev server found". - No behavioral change — the guard's condition, the
nilreturn and every caller are untouched; theLogger.debug(logLevel:scope:message:)call matches the signature and style of the other log sites in this file and inDevMode. Nothing new to test.
All nine inline threads across my prior reviews are now resolved. The body-level observations from the initial review (the app-wide NSAllowsArbitraryLoadsInWebContent guidance, and the absence of coverage for DevServerLocator.locate / applyDevServerOverrideIfNeeded / applyDefaultTestModeState) are unchanged by this delta and remain the humans' call — I am not re-raising them here.
Claude Opus | 𝕏

Changes in this pull request
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.swiftlintin the main directory and fixed any issues.