-
Notifications
You must be signed in to change notification settings - Fork 249
Pass Storefront API requests through the dev proxy as-is #7377
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@shopify/theme': patch | ||
| --- | ||
|
|
||
| Fix theme dev proxy to support SFAPI requests. |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this gets merged let's get @EvilGenius13 to look at it who has done a bunch of header stuff recently to prevent 401 and 429s. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ const CHECKOUT_PATTERN = /^\/checkouts\/(?!internal\/)/ | |||||||||||||||||||||||||
| const ACCOUNT_PATTERN = /^\/account(\/login\/multipass(\/[^/]+)?|\/logout)?\/?$/ | ||||||||||||||||||||||||||
| const VANITY_CDN_PATTERN = new RegExp(`^${VANITY_CDN_PREFIX}`) | ||||||||||||||||||||||||||
| const EXTENSION_CDN_PATTERN = new RegExp(`^${EXTENSION_CDN_PREFIX}`) | ||||||||||||||||||||||||||
| const STOREFRONT_API_PATTERN = /^\/api\/(unstable|\d{4}-\d{2})\/graphql\.json/ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const IGNORED_ENDPOINTS = [ | ||||||||||||||||||||||||||
| '/.well-known', | ||||||||||||||||||||||||||
|
|
@@ -118,6 +119,16 @@ function getStoreFqdnForRegEx(ctx: DevServerContext) { | |||||||||||||||||||||||||
| return ctx.session.storeFqdn.replace(/\\/g, '\\\\').replace(/\./g, '\\.') | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Whether the request should be forwarded to SFR without modification. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| function isPassthroughRequest(event: H3Event) { | ||||||||||||||||||||||||||
| // Forward Storefront API requests as-is. The public Storefront API expects | ||||||||||||||||||||||||||
| // X-Shopify-Storefront-Access-Token from the caller and rejects our SFR | ||||||||||||||||||||||||||
| // devtools bearer, so we must not inject theme auth, cookies, or dev params. | ||||||||||||||||||||||||||
| return STOREFRONT_API_PATTERN.test(event.path) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Replaces every VanityCDN-like (...myshopify.com/cdn/...) URL to pass through the local server. | ||||||||||||||||||||||||||
| * It also replaces MainCDN-like (cdn.shopify.com/...) URLs to files that are known local assets. | ||||||||||||||||||||||||||
|
|
@@ -306,41 +317,39 @@ export function proxyStorefrontRequest(event: H3Event, ctx: DevServerContext): P | |||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // When a .css.liquid or .js.liquid file is requested but it doesn't exist in SFR, | ||||||||||||||||||||||||||
| // it will be rendered with a query string like `assets/file.css?1234`. | ||||||||||||||||||||||||||
| // For some reason, after refreshing, this rendered URL keeps the wrong `?1234` | ||||||||||||||||||||||||||
| // query string for a while. We replace it with a proper timestamp here to fix it. | ||||||||||||||||||||||||||
| if (/\/assets\/[^/]+\.(css|js)$/.test(url.pathname) && /\?\d+$/.test(url.search)) { | ||||||||||||||||||||||||||
| url.search = `?v=${Date.now()}` | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| url.searchParams.set('_fd', '0') | ||||||||||||||||||||||||||
| url.searchParams.set('pb', '0') | ||||||||||||||||||||||||||
| const headers = getProxyStorefrontHeaders(event) | ||||||||||||||||||||||||||
| const body = getRequestWebStream(event) | ||||||||||||||||||||||||||
| let headers = getProxyStorefrontHeaders(event) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (!isPassthroughRequest(event)) { | ||||||||||||||||||||||||||
| // When a .css.liquid or .js.liquid file is requested but it doesn't exist in SFR, | ||||||||||||||||||||||||||
| // it will be rendered with a query string like `assets/file.css?1234`. | ||||||||||||||||||||||||||
| // For some reason, after refreshing, this rendered URL keeps the wrong `?1234` | ||||||||||||||||||||||||||
| // query string for a while. We replace it with a proper timestamp here to fix it. | ||||||||||||||||||||||||||
| if (/\/assets\/[^/]+\.(css|js)$/.test(url.pathname) && /\?\d+$/.test(url.search)) { | ||||||||||||||||||||||||||
| url.search = `?v=${Date.now()}` | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const baseHeaders: Record<string, string> = { | ||||||||||||||||||||||||||
| ...headers, | ||||||||||||||||||||||||||
| ...defaultHeaders(), | ||||||||||||||||||||||||||
| referer: url.origin, | ||||||||||||||||||||||||||
| Cookie: buildCookies(ctx.session, {headers}), | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| url.searchParams.set('_fd', '0') | ||||||||||||||||||||||||||
| url.searchParams.set('pb', '0') | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Only include Authorization for theme dev, not theme-extensions | ||||||||||||||||||||||||||
| if (ctx.type === 'theme') { | ||||||||||||||||||||||||||
| baseHeaders.Authorization = `Bearer ${ctx.session.storefrontToken}` | ||||||||||||||||||||||||||
| headers = cleanHeader({ | ||||||||||||||||||||||||||
| ...headers, | ||||||||||||||||||||||||||
| ...defaultHeaders(), | ||||||||||||||||||||||||||
| referer: url.origin, | ||||||||||||||||||||||||||
| Cookie: buildCookies(ctx.session, {headers}), | ||||||||||||||||||||||||||
| // Only include Authorization for theme dev, not theme-extensions | ||||||||||||||||||||||||||
| ...(ctx.type === 'theme' ? {Authorization: `Bearer ${ctx.session.storefrontToken}`} : {}), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| }) | |
| }) | |
| } else { | |
| delete headers.cookie | |
| delete headers.Cookie | |
| delete headers.authorization | |
| delete headers.Authorization | |
| delete headers.referer | |
| delete headers.Referer | |
| url.searchParams.delete('_fd') | |
| url.searchParams.delete('pb') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The passthrough tests currently assert
headers.Cookie/headers.Authorizationare undefined, butgetProxyStorefrontHeaders()produces lowercased keys (see existing snapshot in this file), so these assertions don’t prove that cookies/authorization aren’t being forwarded (they could still be present asheaders.cookie/headers.authorization). If the goal is to ensure nothing auth/cookie-like is forwarded for SFAPI, update the test to setcookie/referer/authorizationon the incoming request and assert both casings are absent (or, if forwarding caller auth is intended, clarify expectations and ensure the proxy doesn’t overwrite it).