Skip to content

Prepend application url to app proxy url if it starts with / #6147

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.

Sign up for GitHub

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

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

Prepend application url to app proxy url if it starts with / #6147

wants to merge 1 commit into from

Conversation

murray-andrew-r
Copy link

@murray-andrew-r murray-andrew-r commented Jul 22, 2025 •
edited
Loading

WHY are these changes introduced?

Closes https://github.com/shop/issues-develop/issues/12741#issue-3249208952

This puts us at parity with webhook subscription URLs, which already have this feature.

WHAT is this pull request doing?

Appends the application url to the start of the proxy URL if the proxy URL starts with / .

Updates validations to allow / leading proxy URLs. Also ensures that only HTTP proxies are used.

How to test your changes?

  1. pnpm shopify app init
  2. In app.toml add
    [app_proxy]
    url = "https://www.api.example.com/api/proxy2"
    subpath = "store-pickup"
    prefix = "apps"

Substituting focused-share-holder app with your app name:

  1. pnpm shopify app deploy --path=./focused-shareholder-app should succeed
  2. In dev dash, confirm that the app proxy URL is https://www.api.example.com/api/proxy2
  3. Update url to http://www.api.example.com/api/proxy2
  4. pnpm shopify app deploy --path=./focused-shareholder-app should fail with an HTTP validation.
  5. Update url to /api/proxy2
  6. pnpm shopify app deploy --path=./focused-shareholder-app should succeed
  7. In dev dash, confirm that the proxy url is https://example/com/api/proxy2

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

@murray-andrew-r murray-andrew-r force-pushed the prepend-app-url-to-app-proxy branch 2 times, most recently from b159331 to 2f54132 Compare July 23, 2025 13:29
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 23, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
77.96% (-0.02% 🔻)
 13019/16699
🟡 Branches
72.15% (+0.06% 🔼)
 6351/8802
🟡 Functions
78.13% (+0% 🔼)
 3376/4321
🟡 Lines
78.37% (-0.02% 🔻)
 12328/15731
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🟢
... / url_prepender.ts
 100% 100% 100% 100%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / loader.ts
93.36% (-0.03% 🔻)
85.92% (-0.14% 🔻)
 97.27%
94.34% (-0.03% 🔻)
🟡
... / extension-instance.ts
78.13% (-4.17% 🔻)
74.29% (-2.14% 🔻)
91.07% (-1.79% 🔻)
77.53% (-4.49% 🔻)
🟢
... / app_config_webhook_subscription.ts
 100%
50% (-16.67% 🔻)
 100% 100%

Test suite run success

3056 tests passing in 1317 suites.

Report generated by 🧪jest coverage report action from 8cc6411

@murray-andrew-r murray-andrew-r force-pushed the prepend-app-url-to-app-proxy branch 2 times, most recently from b0fc143 to b4d4f97 Compare July 23, 2025 14:08
@murray-andrew-r murray-andrew-r marked this pull request as ready for review July 23, 2025 14:18
@murray-andrew-r murray-andrew-r requested a review from a team as a code owner July 23, 2025 14:18
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@murray-andrew-r murray-andrew-r force-pushed the prepend-app-url-to-app-proxy branch from b4d4f97 to 8cc6411 Compare July 23, 2025 14:28
@murray-andrew-r murray-andrew-r requested a review from a team as a code owner July 23, 2025 14:28
isaacroldan
isaacroldan requested changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Let's investigate if this can be done server side

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

@jackseok jackseok Awaiting requested review from jackseok

@JonathanJChang JonathanJChang Awaiting requested review from JonathanJChang

@pt2pham pt2pham Awaiting requested review from pt2pham

@isaacroldan isaacroldan Awaiting requested review from isaacroldan

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@murray-andrew-r @isaacroldan