Skip to content

feat: add dev mode for previewing local paywalls - #511

Open
chroxify wants to merge 7 commits into
developfrom
christo/sw-framework-support
Open

feat: add dev mode for previewing local paywalls#511
chroxify wants to merge 7 commits into
developfrom
christo/sw-framework-support

Conversation

@chroxify

Copy link
Copy Markdown
Contributor

Changes in this pull request

Checklist

  • All unit tests pass.
  • All UI tests pass.
  • Demo project builds and runs on iOS.
  • Demo project builds and runs on Mac Catalyst.
  • Demo project builds and runs on visionOS.
  • I added/updated tests or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have run swiftlint in the main directory and fixed any issues.
  • I have updated the SDK documentation as well as the online docs.
  • I have reviewed the contributing guide

@yusuftor
yusuftor marked this pull request as ready for review August 25, 2026 17:00
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown

PR author is not in the allowed authors list.

yusuftor and others added 3 commits August 25, 2026 19:02
…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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  • DevMode gateisActive(_:) requires devMode/devServerURL AND DeviceHelper.isSandboxEnvironment (new), so an App Store production build ignores the option and logs a one-time warning.
  • DevServerManifest / DevServerLocator — decodes GET {base}/device/manifest.json, resolves a dashboard paywall to a local surface via a superwall.lock binding (or the single-paywall fallback), and walks localhost:6100…6104 unless devServerURL is set, with a 2s hit cache and 5s miss debounce.
  • Paywall overrideapplyDevServerOverrideIfNeeded rewrites url/urlConfig and nils manifest on every non-debugger paywall response; Paywall's stub init becomes internal and urlConfig/manifest become var.
  • superwall_dev deep 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 pickerpressedPreview now 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 DevMode production gate, and the synthesised Paywall.

⚠️ 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.md states this project uses Swift's Testing framework (@Test/#expect) for all unit tests and that new tests should always use it.
  • DevModeTests mutates the global DevMode.isSandboxEnvironment and restores it in tearDown. Several suites here already flake under parallel execution for exactly this reason — worth knowing if these start failing only in parallel runs.
  • DebugViewController.loadDevServerPreview calls DevServerLocator.shared.locate(...) again even though devServer was already resolved by ensureDevServer and carries the same base; only location.manifest is used from the second call.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/DevServer/DevServerPreview.swift Outdated
Comment thread Sources/SuperwallKit/DevServer/DevServerPreview.swift
Comment thread Sources/SuperwallKit/Debug/DebugViewController.swift Outdated
Comment thread Sources/SuperwallKit/DevServer/DevServerManifest.swift
Comment thread CHANGELOG.md Outdated
Comment thread Sources/SuperwallKit/Debug/DebugViewController.swift Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 ## Unreleased heading is gone; the dev-mode entry now sits under ## 4.17.0 as ### Enhancements, above the fixes that were previously staged as 4.16.4.
  • Version bumpConstants.swift (still on line 21), SuperwallKit.podspec and CHANGELOG.md all read 4.17.0 consistently.
  • project.pbxproj regenerated — wholesale identifier churn from xcodegen; the new DevServer/ 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).

Pullfrog  | Fix it ➔View workflow run | Using 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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 productionDevServerPreview.handle now checks DevMode.isActive synchronously and returns false (still logging the warning) when dev mode is off, and DeepLinkRouter.isSuperwallURL claims the link only once Superwall.isInitialized and canHandle both agree. storeDeepLink runs both pre-configure and configured-but-not-yet-loaded, so the isInitialized gate is not vacuous.
  • Deep-link origin allowlistedisTrustedBase restricts a deep-link-supplied base to loopback, ::1, *.local, RFC1918/link-local ranges, or an exact scheme+host+port match on options.devServerURL. The four-numeric-octet parse correctly rejects 10.0.0.1.evil.example.com, and userinfo smuggling, case, trailing-dot FQDNs and decimal/octal IP forms all fail closed.
  • withTimeout rewritten as a real race — an AsyncStream whose 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? around Task.sleep guarantees the timeout branch always yields, and AsyncStream.Continuation is documented idempotent.
  • Dev-mode cache unfrozensaveRequestHash skips the paywallsByHash memoisation while dev mode is active (concurrent work still dedupes through activeTasks), and the override folds the mount URL into Paywall.cacheKey so a moved server or a published fallback takes .replacePaywall/.loadWebView instead of .updatePaywall.
  • Comparator and lint fixes — the non-strict-weak-ordering sort predicate in DevServerLocator became removeAll/insert, matching the pinnedBase block above it, and the trailing whitespace at DebugViewController.swift:128 is gone.
  • Tests — a new Swift Testing DevServerPreviewTests covering the trusted and untrusted host lists, devServerURL matching and canHandle, plus a DeepLinkRouterTests case that does fail against the pre-fix code.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/DevServer/DevServerManifest.swift
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

pullfrog Bot commented Aug 25, 2026

Copy link
Copy Markdown

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.

Pullfrog  | Rerun failed job ➔View workflow run | via Pullfrog | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 originDevServerManifest.mountURL(for:base:) now returns nil unless the resolved scheme, host and port all equal base'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 addedtest_mountUrlRejectsSurfacesPointingOffTheDevServerOrigin asserts nil for absolute, protocol-relative and other-port surface urls. All three resolve non-nil against 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 URL semantics: a relative surface.url inherits base'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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/DevServer/DevServerManifest.swift
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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 traceableDevServerManifest.mountURL(for:base:) emits a .warn on .superwallCore before returning nil from the origin-pin guard, naming the surface id, its raw url, the resolved absolute URL and base. Both callers still treat nil as a silent no-op, but the nil itself now always carries a log from inside mountURL, which was the ask.
  • Diagnostic quality is right for the failure it covers — the message distinguishes the raw surface.url from what it resolved to, so the representation mismatches this guard can hit (localhost vs 127.0.0.1, or a devServerURL given without a port against an absolute :6100 surface url) read straight out of the log rather than looking like "no dev server found".
  • No behavioral change — the guard's condition, the nil return and every caller are untouched; the Logger.debug(logLevel:scope:message:) call matches the signature and style of the other log sites in this file and in DevMode. 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.

Pullfrog  | View workflow run | Using Claude Opus𝕏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants