Skip to content

fix SSRF vulnerability in proxy.ts #6794

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
FKauwe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix SSRF vulnerability in proxy.ts #6794

FKauwe wants to merge 1 commit into from
+33 −1

Conversation

@FKauwe
Copy link

@FKauwe FKauwe commented Jan 23, 2026 •
edited
Loading

WHY are these changes introduced?

Fixes #947

WHAT is this pull request doing?

Fixes the vulnerability when proxying by introducing a check where if the two host names don't match, the request is not executed and a 502 error is surfaced to the user.
I could have identified hosts preceded by // and then performed the matching logic or even tried to correct the incorrect host but we (I and Josh) chose to throw an error because it's simpler to do, and the alternate method of rewriting the // path would involve regexes, and we don't want to try loading these requests. This change ensures we block the request. We also chose to throw an error instead of outputWarn as this is a rare but real vulnerability and if it happens, we want the surfaced error message to be loud

Baseline behavior:

Malicious request in logs and browser:
Screenshot 2026-01-22 at 3 45 10 PM
Screenshot 2026-01-22 at 3 45 42 PM

How to test your changes?

p build (in editor terminal) on main branch
Start the dev server in an external terminal using:

When the server is running, use the t shortcut to trigger a legitimate request and see the healthy logs output
To trigger the SSRF detection, visit the malicious URL directly in your browser:
http://127.0.0.1:9292//pokeapi.co/api/v2/pokemon/gengar
and see the browser behavior change

Then switch to my branch to see the fix in action:
before testing the new build, kill the server in the external terminal ctrl-c
Then p build
Then restart the server again

Then run the malicious request again and see the new browser and log behavior:
Screenshot 2026-01-22 at 6 47 33 PM
Screenshot 2026-01-22 at 9 00 46 PM
Screenshot 2026-01-22 at 9 05 32 PM
Screenshot 2026-01-22 at 9 06 07 PM

wrote a new unittest to verify the behavior and all proxy tests pass and all storefront-renderer tests pass

@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2026 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
79.52% (+0.29% 🔼)
 14340/18033
🟡 Branches
73.73% (+0.63% 🔼)
 7080/9602
🟡 Functions
79.64% (+0.27% 🔼)
 3665/4602
🟡 Lines
79.88% (+0.3% 🔼)
 13557/16971
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🟢
... / admin-as-app.ts
 100% 100% 100% 100%
🟢
... / metafield_definitions.ts
 100% 100% 100% 100%
🟢
... / metaobject_definitions.ts
 100% 100% 100% 100%
🟢
... / bulk-operation-cancel.ts
 100% 100% 100% 100%
🟢
... / bulk-operation-run-mutation.ts
 100% 100% 100% 100%
🟢
... / bulk-operation-run-query.ts
 100% 100% 100% 100%
🟢
... / get-bulk-operation-by-id.ts
 100% 100% 100% 100%
🟢
... / list-bulk-operations.ts
 100% 100% 100% 100%
🟢
... / staged-uploads-create.ts
 100% 100% 100% 100%
🟢
... / fetch_store_by_domain.ts
 100% 100% 100% 100%
🔴
... / import-custom-data-definitions.ts
 0% 100% 0% 0%
🔴
... / cancel.ts
 0% 100% 0% 0%
🔴
... / execute.ts
 0% 0% 0% 0%
🔴
... / status.ts
 0% 0% 0% 0%
🔴
... / pull.ts
 0% 100% 0% 0%
🟡
... / execute-operation.ts
 75% 50% 100% 75%
🔴
... / pull.ts
 0% 0% 0% 0%
🟢
... / bulk-operation-status.ts
 96.55% 92.11% 100% 100%
🟢
... / cancel-bulk-operation.ts
 100% 100% 100% 100%
🟢
... / constants.ts
 100% 100% 100% 100%
🟢
... / download-bulk-operation-results.ts
 100% 100% 100% 100%
🟢
... / execute-bulk-operation.ts
 92.06% 86.05% 100% 93.55%
🟢
... / format-bulk-operation-status.ts
 100% 100% 100% 100%
🟢
... / run-mutation.ts
 100% 100% 100% 100%
🟢
... / run-query.ts
 100% 100% 100% 100%
🟡
... / stage-file.ts
 73.53% 62.5% 85.71% 72.73%
🟢
... / watch-bulk-operation.ts
 100% 94.74% 100% 100%
🟢
... / declarative-definitions.ts
 98.54% 93.18% 100% 98.51%
🟢
... / common.ts
 97.62% 95% 100% 97.06%
🟢
... / execute-command-helpers.ts
 100% 100% 100% 100%
🟢
... / file-formatter.ts
 100% 100% 100% 100%
🔴
... / promiseWithResolvers.ts
 33.33% 50% 50% 33.33%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🔴
... / execute.ts
 0%
0% (-100% 🔻)
 0% 0%
🟢
... / loader.ts
94.06% (+0.2% 🔼)
86.41% (-0.42% 🔻)
97.17% (+0.11% 🔼)
94.85% (+0.18% 🔼)
🟢
... / extension-instance.ts
84.8% (+0.23% 🔼)
77.6% (-0.91% 🔻)
92.06% (+0.13% 🔼)
85.11% (+0.24% 🔼)
🟡
... / specification.ts
69.64% (+0.55% 🔼)
75.61% (+2.44% 🔼)
76.47% (-1.31% 🔻)
69.39% (+0.64% 🔼)
🟢
... / ui_extension.ts
87.9% (-6.93% 🔻)
77.19% (-4.06% 🔻)
85.19% (-14.81% 🔻)
90.07% (-6.39% 🔻)
🟢
... / store-context.ts
 100%
82.35% (-0.98% 🔻)
 100% 100%
🟢
... / Logs.tsx
 90%
90.91% (-5.97% 🔻)
 100% 90%
🟢
... / fetch.ts
84.21% (+0.88% 🔼)
82.35% (-0.98% 🔻)
 75%
85.29% (+1.42% 🔼)
🟢
... / app-event-watcher-handler.ts
86.36% (-4.11% 🔻)
 75% 86.67%
85.71% (-5.19% 🔻)
🟢
... / bundle.ts
 93.22%
63.33% (-3.33% 🔻)
94.12% (+5.88% 🔼)
 96.3%
🟢
... / developer-platform-client.ts
84.62% (-1.5% 🔻)
71.43% (+0.84% 🔼)
81.82% (+1.82% 🔼)
93.75% (+0.42% 🔼)
🔴
... / http-reverse-proxy.ts
58.97% (-4.91% 🔻)
37.04% (-2.96% 🔻)
58.33% (-5.3% 🔻)
59.46% (-5.25% 🔻)
🔴
... / app-management-client.ts
53.69% (-0.16% 🔻)
 47.47%
50% (-0.45% 🔻)
52.53% (-0.17% 🔻)
🟢
... / api.ts
87.07% (-0.43% 🔻)
76.71% (-0.1% 🔻)
 100%
86.49% (-0.43% 🔻)
🟢
... / SingleTask.tsx
84.21% (-15.79% 🔻)
50% (-50% 🔻)
80% (-20% 🔻)
84.21% (-15.79% 🔻)
🔴
... / environment.ts
35% (-5% 🔻)
 41.18%
40% (-10% 🔻)
36.84% (-5.26% 🔻)
🔴
... / ui.tsx
50.82% (-0.79% 🔻)
42.86% (-5.53% 🔻)
54.55% (+1.42% 🔼)
50% (-0.82% 🔻)
🟢
... / console.ts
81.82% (+15.15% 🔼)
75% (-25% 🔻)
100% (+33.33% 🔼)
81.82% (+15.15% 🔼)
🟢
... / init.ts
88% (-0.89% 🔻)
71.43% (+4.76% 🔼)
86.67% (+4.85% 🔼)
88% (-0.89% 🔻)
🟢
... / storefront-renderer.ts
90.2% (-0.54% 🔻)
 78.95%
81.82% (-1.52% 🔻)
90.2% (-0.54% 🔻)
🟡
... / theme-polling.ts
67.12% (-0.93% 🔻)
 68.75% 78.57%
66.67% (-0.98% 🔻)

Test suite run success

3658 tests passing in 1429 suites.

Report generated by 🧪jest coverage report action from 951f207

@FKauwe FKauwe self-assigned this Jan 23, 2026
@FKauwe FKauwe marked this pull request as ready for review January 23, 2026 21:04
@FKauwe FKauwe requested review from a team as code owners January 23, 2026 21:04
@FKauwe FKauwe requested a review from EvilGenius13 January 23, 2026 21:05
EvilGenius13
EvilGenius13 reviewed Jan 26, 2026
View reviewed changes
@FKauwe
Copy link
Author

FKauwe commented Jan 28, 2026 •
edited
Loading

@EvilGenius13 pointed out there is conditional branching logic in the proxyStorefrontRequest function when the host is being set by the event.path object and I was only testing one path.
I understood the need to have test coverage for both branches of the conditional logic but didn't understand exactly what the conditions were or how the urls were being built based on the presence or absence of the prefix. I dug into this, here are my learnings:
createH3Event() is a function in the proxy.ts.test file that creates a fake HTTP request event. The second argument becomes event.path — that's the URL path someone is requesting and event.path is used in the proxyStorefrontRequest function.

These are the questions I identified I need to answer in my investigation:
what is the difference between this: createH3Event('GET', '//evil.com/some-path')
and this: createH3Event('GET', '/ext/cdn//evil.com/some-path')
and how does that relate to: const host = event.path.startsWith(EXTENSION_CDN_PREFIX) ? 'cdn.shopify.com' : ctx.session.storeFqdn

And the answers I came up:
The second one: '/ext/cdn//[evil.com/some-path](http://evil.com/some-path)' since it is a different path, will force an execution of the other ternary branching logic where the condition: event.path.startsWith(EXTENSION_CDN_PREFIX) is found to be TRUE
Both paths will go through the workflow of the function and two different urls will be created, which are:

new URL('//evil.com/some-path', 'https://my-store.myshopify.com')
Result URL: https://evil.com/some-path
url.hostname = 'evil.com'

(for the path without the ext)
And:

new URL('//evil.com/some-path', 'https://cdn.shopify.com')
Result URL: https://evil.com/some-path
url.hostname = 'evil.com'
For the path *with the ext

Both paths will ultimately fail in the new check that I wrote, since in both cases, the hostname will be ‘evil.com’ which will not match the respective hosts of the urls, which are my-store.myshopify.com , cdn.shopify.com but the mismatches will be caused by two different hosts
I added the new test to cover both hosts being created by the conditional logic and I renamed them to make it clear that the prefix name and the host mismatching are being tested.

@FKauwe FKauwe force-pushed the josh-faith-pairing-01-20 branch from d526b32 to 951f207 Compare January 28, 2026 04:50
@FKauwe FKauwe requested a review from graygilmore January 28, 2026 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EvilGenius13 EvilGenius13 EvilGenius13 left review comments

@graygilmore graygilmore Awaiting requested review from graygilmore

At least 1 approving review is required to merge this pull request.

Assignees

@FKauwe FKauwe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@FKauwe @EvilGenius13