Skip to content

Fix: Allow compliance webhooks with legacy install flow #6108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmerand
Copy link
Contributor

@dmerand dmerand commented Jul 14, 2025

WHY are these changes introduced?

Fixes #6003 regression - After PR #6003, users reported that the CLI incorrectly prevents deployment when using only compliance webhooks with use_legacy_install_flow = true.

The validation treats ALL webhook subscriptions as app-specific, including compliance topics which are mandatory for public apps and should be allowed with legacy install flow.

User report: https://community.shopify.dev/t/compliance-topics-issue/18702

WHAT is this pull request doing?

This PR fixes the webhook validation logic to properly distinguish between app-specific webhooks and compliance webhooks:

  • Before: The validation counted ALL webhook subscriptions as app-specific
  • After: The validation only counts subscriptions with the topics field as app-specific

The fix allows compliance-only webhook configurations to work with legacy install flow, which is necessary because:

  1. Compliance webhooks are mandatory for all public apps (GDPR requirement)
  2. They use the compliance_topics field, not the topics field
  3. They don't require declarative scopes like regular app-specific webhooks do

How to test your changes?

  1. Create an app with use_legacy_install_flow = true
  2. Add only compliance webhooks:
    [webhooks]
    api_version = "2025-07"
    
    [[webhooks.subscriptions]]
    uri = "/webhooks"
    compliance_topics = ["customers/data_request", "customers/redact", "shop/redact"]
  3. Run shopify app deploy - it should now succeed
  4. Add a regular webhook with topics - deployment should fail as expected

Post-release steps

None required.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

dmerand commented Jul 14, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
78.35% (-0.05% 🔻)
13034/16636
🟡 Branches
72.59% (+0.02% 🔼)
6347/8744
🟡 Functions
78.23% (-0.05% 🔻)
3375/4314
🟡 Lines
78.76% (-0.05% 🔻)
12343/15671
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / shop_details.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / index.ts
0% (-6.67% 🔻)
0% 0%
0% (-7.14% 🔻)
🔴
... / current_user_account.ts
0% (-100% 🔻)
100% 100%
0% (-100% 🔻)
🟢
... / copy.ts
97.06% (-0.08% 🔻)
94.29% 100%
97.06% (-0.08% 🔻)
🔴
... / mock-api-client.ts
3.7% (-0.64% 🔻)
0%
9.09% (-0.91% 🔻)
4% (-0.55% 🔻)
🟢
... / store-import.ts
97.78% (-0.09% 🔻)
85.71% (+5.71% 🔼)
100%
97.73% (-0.1% 🔻)

Test suite run success

3032 tests passing in 1317 suites.

Report generated by 🧪jest coverage report action from 7974965

@dmerand dmerand marked this pull request as ready for review July 14, 2025 19:02
@dmerand dmerand requested review from a team as code owners July 14, 2025 19:02
@dmerand dmerand marked this pull request as draft July 14, 2025 19:07
@dmerand dmerand marked this pull request as ready for review July 14, 2025 19:26
await expect(app.preDeployValidation()).resolves.not.toThrow()
})

test('does not throw an error for subscription with only compliance_topics and no topics field', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this test exactly the same as the first one? or am i missing something?

@isaacroldan
Copy link
Contributor

Do we need a patch release for this?

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