Skip to content

feat(PUSH-834): loosen open_url scheme gate to allowlist#623

Draft
morrilltim wants to merge 4 commits into
feat/push-638-open-urlfrom
timmorrill/push-834-handle-non-web-url-schemes
Draft

feat(PUSH-834): loosen open_url scheme gate to allowlist#623
morrilltim wants to merge 4 commits into
feat/push-638-open-urlfrom
timmorrill/push-834-handle-non-web-url-schemes

Conversation

@morrilltim

Copy link
Copy Markdown

Summary

Loosen the open_url action's http/https gate to an allowlist supporting mailto:, tel:, sms:, smsto: in addition to web schemes. No dispatch changes needed (UIApplication.shared.open is already scheme-agnostic).

Changes

  • Loosen three input gates (DeepLinkHandler, UNNotificationResponse+Klaviyo, KlaviyoActionButtonParser) to accept allowlist
  • Add shared URLSchemeAllowlist constant (package visibility in KlaviyoCore; internal mirror in KlaviyoSwiftExtension due to module dependencies)
  • Block intent:, android-app:, javascript:, file:, content:, data: (scheme validation)
  • On blocked scheme: drop silently (existing nil-return fall-through behavior)

Test Plan

  • Full test suite passes: 250 tests, 0 failures
  • New tests: allowlisted schemes accepted, blocked schemes rejected, .openWebUrl dispatched for mailto:/tel:
  • Existing scheme-rejection tests updated

Related

Part of PUSH-834 (multi-repo). Also see Android and fender PRs.

https://linear.app/klaviyo/issue/PUSH-834

morrilltim and others added 2 commits June 25, 2026 09:46
…, sms, smsto)

Define openUrlAllowedSchemes in KlaviyoCore (and mirror it in KlaviyoSwiftExtension,
which cannot depend on KlaviyoCore due to NSE sandbox). Update klaviyoWebUrl and
isValidActionURLCombination to check the allowlist instead of the hard-coded
["http","https"] set. Dangerous schemes (intent, javascript, file, etc.) continue
to be silently dropped.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- UNNotificationResponseExtensionTests: flip mailto: test to expect URL (not nil);
  add tel:, sms:, smsto: acceptance tests; add intent:, javascript:, file: rejection tests
- KlaviyoActionButtonParserTests: rename SkipsOpenUrlWithNonHttpScheme (removed mailto:
  from blocked list) to SkipsOpenUrlWithBlockedScheme; add acceptance tests for
  mailto:, tel:, sms:; add rejection tests for intent:, javascript:
- DeepLinkHandlingTests: add dispatch tests confirming .openWebUrl is sent for mailto:
  and tel: web_url values; add test confirming blocked schemes (javascript:) are dropped

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Enterprise

Run ID: 49e1697c-5e6e-49ff-aec3-9cc0c5708a32

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch timmorrill/push-834-handle-non-web-url-schemes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

⚠️ This PR needs to target a release branch before it can merge to master.

All changes — including hotfixes, dependency bumps, and feature work — should flow through a rel/* branch rather than merging directly to master. This keeps commit history clean and ensures every change is tied to a release.

To fix: change the base branch of this PR to the appropriate rel/* branch when available. You can do this without closing the PR — use the base branch dropdown at the top of the PR.

  • If this is CI/tooling work, rename your branch with a ci/ prefix and this check will pass
  • If this is documentation-only work, rename your branch with a docs/ prefix and this check will pass

Override: In exceptional circumstances, a maintainer may add the manual-merge label to bypass this check and merge directly to master. This should be used sparingly — it skips the release branch entirely and may result in changes not being included in a release or loss of commit attribution if not handled carefully.

Enforced by the pr-target-check workflow.

@github-actions github-actions Bot added the needs: release branch PR must be retargeted to a rel/* branch before merging to master label Jun 25, 2026

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c49bf4c. Configure here.

Comment thread Sources/KlaviyoSwift/Klaviyo.swift
Comment thread Tests/KlaviyoSwiftExtensionTests/KlaviyoActionButtonParserTests.swift Outdated
@morrilltim morrilltim changed the base branch from master to feat/push-638-open-url June 25, 2026 14:09
morrilltim and others added 2 commits June 25, 2026 10:15
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Action button open_url taps dispatched .openWebUrl without applying the
scheme allowlist, so a blocked scheme could reach UIApplication.shared.open
when the NSE skipped parser validation (non-empty categoryIdentifier).
Apply openUrlAllowedSchemes at dispatch; blocked schemes are dropped with a
warning while the tap event is still tracked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: release branch PR must be retargeted to a rel/* branch before merging to master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant