diff --git a/.changeset/graphql-url-parse-warning.md b/.changeset/graphql-url-parse-warning.md new file mode 100644 index 00000000000..54d93d2f519 --- /dev/null +++ b/.changeset/graphql-url-parse-warning.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Fix the `url.parse()` deprecation warning during GraphQL requests. diff --git a/packages/app/package.json b/packages/app/package.json index 4c4ae4293db..7945368de35 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -69,7 +69,7 @@ "csv-parse": "7.0.2", "diff": "5.2.2", "esbuild": "0.28.1", - "graphql-request": "6.1.0", + "graphql-request": "7.4.0", "h3": "1.15.11", "http-proxy-node16": "1.0.6", "ignore": "6.0.2", diff --git a/packages/app/src/cli/commands/app/execute.test.ts b/packages/app/src/cli/commands/app/execute.test.ts index edf33597d4a..5330014b833 100644 --- a/packages/app/src/cli/commands/app/execute.test.ts +++ b/packages/app/src/cli/commands/app/execute.test.ts @@ -74,7 +74,7 @@ test('preserves native GraphQL aliases, order, and UTF-8 on stdout and in output expect(response.errors).toBeUndefined() const extensions = JSON.parse('{"__proto__":{"trace":"preserved"},"constructor":false,"nullable":null,"empty":[]}') vi.mocked(adminRequestDoc).mockImplementation(async ({responseOptions}) => { - responseOptions?.onResponse?.({data: response.data, extensions, status: 200, headers: new Headers()}) + responseOptions?.onResponse?.({data: response.data, extensions, status: 200, headers: new Headers(), body: ''}) return response.data }) const json = JSON.stringify({data: response.data, extensions}, null, 2) @@ -126,7 +126,9 @@ test('uses one shared fatal error document and preserves an existing output file data: {shop: null}, extensions: {cost: {actualQueryCost: 1}}, } - vi.mocked(adminRequestDoc).mockRejectedValue(new ClientError({...details, status: 200}, {query})) + vi.mocked(adminRequestDoc).mockRejectedValue( + new ClientError({...details, status: 200, headers: new Headers(), body: ''}, {query}), + ) vi.stubEnv('SHOPIFY_FLAG_JSON', '1') await withCapturedStandardStreams(async ({stdout, stderr}) => { await runCommand(['--json', '--output-file', outputFile]).catch(handler) @@ -141,7 +143,9 @@ test('uses one shared fatal error document and preserves an existing output file test('preserves the text error banner without a fatal error for a GraphQL failure', async () => { const errors = [new GraphQLError('Field invalidField does not exist')] - vi.mocked(adminRequestDoc).mockRejectedValue(new ClientError({errors, status: 200}, {query})) + vi.mocked(adminRequestDoc).mockRejectedValue( + new ClientError({errors, status: 200, headers: new Headers(), body: ''}, {query}), + ) await withCapturedStandardStreams(async ({stdout, stderr}) => { await expect(runCommand([])).resolves.toEqual({app}) expect(stdout()).toBe('') diff --git a/packages/app/src/cli/commands/app/import/custom-data-definitions.test.ts b/packages/app/src/cli/commands/app/import/custom-data-definitions.test.ts index 4f8a1377df0..107f2a0ca1a 100644 --- a/packages/app/src/cli/commands/app/import/custom-data-definitions.test.ts +++ b/packages/app/src/cli/commands/app/import/custom-data-definitions.test.ts @@ -68,7 +68,7 @@ async function withApp(run: (directory: string) => Promise) { metafieldDefinitions: { pageInfo: {hasNextPage: false, endCursor: null}, nodes: - variables?.ownerType === 'PRODUCT' + variables && 'ownerType' in variables && variables.ownerType === 'PRODUCT' ? [ { key: 'color', @@ -127,7 +127,13 @@ test.each([ test.each([false, true])('distinguishes an empty conversion from denied scope: %s', async (scopeDenied) => { await withApp(async (directory) => { vi.mocked(adminAsAppRequestDoc).mockImplementation(async ({query, variables}) => { - if (scopeDenied && query === MetafieldDefinitions && variables?.ownerType === 'PRODUCT') { + if ( + scopeDenied && + query === MetafieldDefinitions && + variables && + 'ownerType' in variables && + variables.ownerType === 'PRODUCT' + ) { throw new Error('ACCESS_DENIED: Missing access scope') } return { diff --git a/packages/app/src/cli/commands/app/release.test.ts b/packages/app/src/cli/commands/app/release.test.ts index e24d4773300..b08c68dd0b6 100644 --- a/packages/app/src/cli/commands/app/release.test.ts +++ b/packages/app/src/cli/commands/app/release.test.ts @@ -108,6 +108,7 @@ describe('app release command', () => { rawRequest.mockRejectedValueOnce( new ClientError( { + body: '', status: 200, headers: new Headers(), errors: [ @@ -172,7 +173,12 @@ describe('app release command', () => { const releaseRequest = vi.spyOn(client, 'release') rawRequest.mockRejectedValueOnce( new ClientError( - {status, headers: new Headers({'x-request-id': 'lookup-request-id'}), errors: [new GraphQLError(message)]}, + { + status, + headers: new Headers({'x-request-id': 'lookup-request-id'}), + body: '', + errors: [new GraphQLError(message)], + }, {query: 'query AppVersionByTag { versionByTag { id } }'}, ), ) diff --git a/packages/app/src/cli/services/execute-operation.test.ts b/packages/app/src/cli/services/execute-operation.test.ts index 29afb35d2ea..70374ba51ea 100644 --- a/packages/app/src/cli/services/execute-operation.test.ts +++ b/packages/app/src/cli/services/execute-operation.test.ts @@ -203,7 +203,7 @@ describe('executeOperation', () => { const data = {store: {name: 'Test Shop'}, value: null, enabled: false} const extensions = {cost: {requestedQueryCost: 2}} vi.mocked(adminRequestDoc).mockImplementation(async ({responseOptions}) => { - responseOptions?.onResponse?.({data, extensions, status: 200, headers: new Headers()}) + responseOptions?.onResponse?.({data, extensions, status: 200, headers: new Headers(), body: ''}) return data }) @@ -220,6 +220,8 @@ describe('executeOperation', () => { test('retains partial query data and native error extensions', async () => { const response = { + headers: new Headers(), + body: '', errors: [new GraphQLError('Access denied', {extensions: {code: 'ACCESS_DENIED'}})], data: {shop: null}, extensions: {cost: {actualQueryCost: 1}}, diff --git a/packages/app/src/cli/services/generate/shop-import/declarative-definitions.test.ts b/packages/app/src/cli/services/generate/shop-import/declarative-definitions.test.ts index 6269ed71c3e..a6dda293ac5 100644 --- a/packages/app/src/cli/services/generate/shop-import/declarative-definitions.test.ts +++ b/packages/app/src/cli/services/generate/shop-import/declarative-definitions.test.ts @@ -1176,7 +1176,12 @@ describe('importDeclarativeDefinitions', () => { }) vi.mocked(adminAsAppRequestDoc).mockImplementation(async ({query, variables}) => { - if (query === MetafieldDefinitions && variables?.ownerType === 'PRODUCT') { + if ( + query === MetafieldDefinitions && + variables && + 'ownerType' in variables && + variables.ownerType === 'PRODUCT' + ) { return { metafieldDefinitions: { pageInfo: {hasNextPage: false, endCursor: null}, diff --git a/packages/app/tsconfig.json b/packages/app/tsconfig.json index 65b0195ab48..f09c3fbddc5 100644 --- a/packages/app/tsconfig.json +++ b/packages/app/tsconfig.json @@ -3,6 +3,7 @@ "include": ["./src/**/*.ts", "./src/**/*.js", "./src/**/*.tsx"], "exclude": ["./dist", "./src/templates/**/*"], "compilerOptions": { + "lib": ["ES2024", "DOM"], "outDir": "dist", "allowJs": true, "rootDir": "src", diff --git a/packages/cli-kit/package.json b/packages/cli-kit/package.json index c7cc524f7c5..9c3cb77b303 100644 --- a/packages/cli-kit/package.json +++ b/packages/cli-kit/package.json @@ -134,7 +134,7 @@ "fs-extra": "11.1.0", "gradient-string": "2.0.2", "graphql": "16.14.2", - "graphql-request": "6.1.0", + "graphql-request": "7.4.0", "h3": "1.15.11", "ignore": "6.0.2", "ink": "6.8.0", diff --git a/packages/cli-kit/src/private/node/api.test.ts b/packages/cli-kit/src/private/node/api.test.ts index ad0922fbb60..4881caeae52 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/packages/cli-kit/src/private/node/api.test.ts @@ -19,6 +19,7 @@ describe('retryAwareRequest', () => { test('handles retries', async () => { // First give a network error; then a rate limit with an explicit retry; then an unknown rate limit; then a successful call const rateLimitedResponseWithRetry = { + body: '', status: 200, errors: [ { @@ -33,6 +34,7 @@ describe('retryAwareRequest', () => { } const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -97,6 +99,7 @@ describe('retryAwareRequest', () => { // whose GraphQL error has extensions.code "THROTTLED" — no 429 status, no // retry-after header. const throttledResponse = { + body: '', status: 200, errors: [ { @@ -152,6 +155,7 @@ describe('retryAwareRequest', () => { // This test gives a false warning from vitest if fake timers are used. It thinks the exception is uncaught. vi.useRealTimers() const messageOnlyResponse = { + body: '', status: 200, errors: [ { @@ -190,6 +194,7 @@ describe('retryAwareRequest', () => { // This test gives a false warning from vitest if fake timers are used. It thinks the exception is uncaught. vi.useRealTimers() const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -476,6 +481,7 @@ describe('retryAwareRequest', () => { test('records retry events when recordRetries is enabled', async () => { const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -531,6 +537,7 @@ describe('retryAwareRequest', () => { test('does not record retry events when recordRetries is disabled', async () => { const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -585,6 +592,7 @@ describe('retryAwareRequest', () => { test('records multiple retry events with correct attempt numbers', async () => { const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -644,6 +652,7 @@ describe('retryAwareRequest', () => { test('records retry events for too many requests status', async () => { const rateLimitedResponse = { + body: '', status: 200, errors: [ { @@ -699,6 +708,19 @@ describe('retryAwareRequest', () => { }) describe('isTransientNetworkError', () => { + test.each([ + Object.assign(new Error('read ECONNRESET'), {code: 'ECONNRESET'}), + Object.assign(new Error('other side closed'), {code: 'UND_ERR_SOCKET'}), + new AggregateError([Object.assign(new Error('connect failed'), {code: 'ECONNREFUSED'})]), + ])('recognizes native fetch network causes: %s', (cause) => { + expect(isTransientNetworkError(new TypeError('fetch failed', {cause}))).toBe(true) + }) + + test('does not retry native fetch configuration errors', () => { + const cause = Object.assign(new Error('invalid keep-alive header'), {code: 'UND_ERR_INVALID_ARG'}) + expect(isTransientNetworkError(new TypeError('fetch failed', {cause}))).toBe(false) + }) + test('identifies transient network errors that should be retried', () => { const transientErrors = [ 'socket hang up', @@ -770,6 +792,12 @@ describe('isTransientNetworkError', () => { }) describe('isNetworkError', () => { + test('recognizes native fetch certificate errors without retrying them', () => { + const error = new TypeError('fetch failed', {cause: new Error('self-signed certificate')}) + expect(isNetworkError(error)).toBe(true) + expect(isTransientNetworkError(error)).toBe(false) + }) + test('identifies all transient network errors', () => { const transientErrors = ['ECONNRESET', 'ETIMEDOUT', 'ENOTFOUND', 'socket hang up', 'premature close'] diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 969bbae3611..581da18544b 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -102,8 +102,9 @@ export function isTransientNetworkError(error: unknown): boolean { 'timeout', 'premature close', 'getaddrinfo', + 'und_err_socket', ] - const errorMessage = error.message.toLowerCase() + const errorMessage = networkErrorMessage(error) const anyMatches = transientErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage)) const missingReason = /^request to .* failed, reason:\s*$/.test(errorMessage) return anyMatches || missingReason @@ -131,13 +132,25 @@ export function isNetworkError(error: unknown): boolean { // Then check for permanent network errors (SSL/TLS/certificate issues) if (error instanceof Error) { const permanentNetworkErrorMessages = ['certificate', 'cert', 'tls', 'ssl', 'altnames'] - const errorMessage = error.message.toLowerCase() + const errorMessage = networkErrorMessage(error) return permanentNetworkErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage)) } return false } +function networkErrorMessage(error: Error): string { + // Native fetch wraps connection and TLS failures in TypeError("fetch failed"). + const cause = + error instanceof TypeError && error.message === 'fetch failed' && error.cause instanceof Error ? error.cause : error + const errors = cause instanceof AggregateError ? cause.errors : [cause] + return errors + .filter((entry): entry is Error => entry instanceof Error) + .map((entry) => `${entry.message} ${'code' in entry ? entry.code : ''}`) + .join(' ') + .toLowerCase() +} + /** * Lower-cased substrings that mean a request was aborted rather than failing on its own. * `node-fetch` says 'The user aborted a request.', undici says 'This operation was aborted', and diff --git a/packages/cli-kit/src/private/node/api/graphql.ts b/packages/cli-kit/src/private/node/api/graphql.ts index 9d99517f271..6c0c5df9224 100644 --- a/packages/cli-kit/src/private/node/api/graphql.ts +++ b/packages/cli-kit/src/private/node/api/graphql.ts @@ -52,7 +52,7 @@ function sanitizeDeepVariables(value: unknown, sensitiveKeys: string[]): unknown return value.map((item) => sanitizeDeepVariables(item, sensitiveKeys)) } - const result: Variables = {} + const result: Record = {} for (const [key, val] of Object.entries(value)) { if (sensitiveKeys.includes(key) && typeof val === 'string') { diff --git a/packages/cli-kit/src/private/node/api/headers.test.ts b/packages/cli-kit/src/private/node/api/headers.test.ts index ca5fc50ae76..cb3360782f0 100644 --- a/packages/cli-kit/src/private/node/api/headers.test.ts +++ b/packages/cli-kit/src/private/node/api/headers.test.ts @@ -24,7 +24,6 @@ describe('common API methods', () => { const version = CLI_KIT_VERSION expect(headers).toEqual({ 'Content-Type': 'application/json', - 'Keep-Alive': 'timeout=30', 'X-Shopify-Access-Token': 'Bearer my-token', 'User-Agent': `Shopify CLI; v=${version}`, authorization: 'Bearer my-token', @@ -45,7 +44,6 @@ describe('common API methods', () => { const version = CLI_KIT_VERSION expect(headers).toEqual({ 'Content-Type': 'application/json', - 'Keep-Alive': 'timeout=30', 'X-Shopify-Access-Token': token, 'User-Agent': `Shopify CLI; v=${version}`, authorization: token, diff --git a/packages/cli-kit/src/private/node/api/headers.ts b/packages/cli-kit/src/private/node/api/headers.ts index c5947d24582..fac1b7a5737 100644 --- a/packages/cli-kit/src/private/node/api/headers.ts +++ b/packages/cli-kit/src/private/node/api/headers.ts @@ -56,7 +56,6 @@ export function buildHeaders(token?: string): Record { const headers: Record = { 'User-Agent': userAgent, - 'Keep-Alive': 'timeout=30', // 'Sec-CH-UA': secCHUA, This header requires the Git sha. 'Sec-CH-UA-PLATFORM': process.platform, 'Content-Type': 'application/json', diff --git a/packages/cli-kit/src/public/node/api/graphql.test.ts b/packages/cli-kit/src/public/node/api/graphql.test.ts index e5da87b4e7c..22cd2c97d6f 100644 --- a/packages/cli-kit/src/public/node/api/graphql.test.ts +++ b/packages/cli-kit/src/public/node/api/graphql.test.ts @@ -14,7 +14,9 @@ import {test, vi, describe, expect, beforeEach, beforeAll, afterAll, afterEach} import {TypedDocumentNode} from '@graphql-typed-document-node/core' import {setupServer} from 'msw/node' -import {graphql, HttpResponse} from 'msw' +import {graphql, http, HttpResponse} from 'msw' +import {ClientError} from 'graphql-request' +import url from 'node:url' let mockedRequestId = 'request-id-123' @@ -131,6 +133,59 @@ afterEach(() => { }) describe('graphqlRequest', () => { + test.each([401, 403, 503])('preserves HTTP %i errors with a non-GraphQL response body', async (status) => { + server.use( + http.post( + mockedAddress, + () => + new HttpResponse('Service unavailable', { + status, + headers: {'Content-Type': 'text/plain', 'x-request-id': 'http-error-request'}, + }), + ), + ) + + const request = graphqlRequest({ + api: 'mockApi', + url: mockedAddress, + query: 'query { ok }', + responseOptions: {handleErrors: false}, + preferredBehaviour: 'non-blocking', + }) + await expect(request).rejects.toBeInstanceOf(ClientError) + await expect(request).rejects.toMatchObject({response: {status, body: 'Service unavailable'}}) + expect(requestIdsCollection.getRequestIds()).toContain('http-error-request') + }) + + test('does not use the deprecated URL parser', async () => { + // Dependencies can retain url.parse, so observe the legacy parser through its prototype. + const parse = vi.spyOn(Reflect.get(url, 'Url').prototype, 'parse') + + try { + await expect( + graphqlRequest({query: 'query QueryName { example }', api: 'mockApi', url: mockedAddress}), + ).resolves.toEqual({QueryName: {example: 'hello'}}) + expect(parse).not.toHaveBeenCalled() + } finally { + parse.mockRestore() + } + }) + + test('rejects an already aborted request without starting it', async () => { + const onRequest = vi.fn() + server.events.on('request:start', onRequest) + + await expect( + graphqlRequest({ + query: 'query QueryName { example }', + api: 'mockApi', + url: mockedAddress, + preferredBehaviour: {useNetworkLevelRetry: false, useAbortSignal: AbortSignal.abort()}, + }), + ).rejects.toMatchObject({name: 'AbortError'}) + expect(onRequest).not.toHaveBeenCalled() + }) + test('calls debugLogRequestInfo once', async () => { let headers: any server.events.on('request:start', ({request}) => { @@ -155,14 +210,9 @@ describe('graphqlRequest', () => { const {'user-agent': userAgent, 'sec-ch-ua-platform': _platform, ...otherHeaders} = headers expect(otherHeaders).toMatchInlineSnapshot(` { - "accept": "*/*", - "accept-encoding": "gzip,deflate", + "accept": "application/graphql-response+json, application/json", "authorization": "Bearer token", - "connection": "close", - "content-length": "100", "content-type": "application/json", - "host": "shopify.example", - "keep-alive": "timeout=30", "some": "header", "x-shopify-access-token": "Bearer token", } diff --git a/packages/cli-kit/src/public/node/api/graphql.ts b/packages/cli-kit/src/public/node/api/graphql.ts index 137bfd1b365..74a51388613 100644 --- a/packages/cli-kit/src/public/node/api/graphql.ts +++ b/packages/cli-kit/src/public/node/api/graphql.ts @@ -1,4 +1,4 @@ -import {buildHeaders, httpsAgent} from '../../../private/node/api/headers.js' +import {buildHeaders} from '../../../private/node/api/headers.js' import {debugLogRequestInfo, errorHandler} from '../../../private/node/api/graphql.js' import {addPublicMetadata, runWithTimer} from '../metadata.js' import {retryAwareRequest} from '../../../private/node/api.js' @@ -16,14 +16,7 @@ import {abortSignalFromRequestBehaviour, RequestBehaviour, requestMode, RequestM import {CLI_KIT_VERSION} from '../../common/version.js' import {sleep} from '../system.js' import {outputContent, outputDebug} from '../output.js' -import { - GraphQLClient, - rawRequest, - RequestDocument, - resolveRequestDocument, - Variables, - ClientError, -} from 'graphql-request' +import {GraphQLClient, rawRequest, RequestDocument, analyzeDocument, Variables, ClientError} from 'graphql-request' import {TypedDocumentNode} from '@graphql-typed-document-node/core' // to replace TVariable type when there graphql query has no variables @@ -101,7 +94,7 @@ export interface GraphQLResponseOptions { const MAX_RATE_LIMIT_RESTORE_DELAY_SECONDS = 0.3 -async function createGraphQLClient({ +function createGraphQLClient({ url, addedHeaders, token, @@ -114,10 +107,8 @@ async function createGraphQLClient({ ...addedHeaders, ...buildHeaders(token), } - const clientOptions = {agent: await httpsAgent(), headers} - return { - client: new GraphQLClient(url, clientOptions), + client: new GraphQLClient(url, {headers}), headers, } } @@ -176,7 +167,7 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti } = options const behaviour = requestMode(options.preferredBehaviour ?? 'default') - let {headers, client} = await createGraphQLClient({url, addedHeaders, token}) + let {headers, client} = createGraphQLClient({url, addedHeaders, token}) debugLogRequestInfo(api, queryAsString, url, variables, headers) const rawGraphQLRequest = async () => { @@ -201,7 +192,7 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti ? async () => { const refreshTokenResult = await tokenRefreshHandler() if (refreshTokenResult.token) { - const {client: newClient, headers: newHeaders} = await createGraphQLClient({ + const {client: newClient, headers: newHeaders} = createGraphQLClient({ url, addedHeaders, token: refreshTokenResult.token, @@ -315,6 +306,6 @@ export async function graphqlRequestDoc( ): Promise { return performGraphQLRequest({ ...options, - queryAsString: resolveRequestDocument(options.query).query, + queryAsString: analyzeDocument(options.query).expression, }) } diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index 2fd5d1a860a..32f6bc5ad22 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -129,6 +129,8 @@ describe('fetchTheme', () => { test('returns undefined when a theme is not found', async () => { const errorResponse = { + headers: new Headers(), + body: '', status: 200, errors: [{message: 'Tema não existe'} as any], } @@ -210,6 +212,8 @@ describe('findDevelopmentThemeByName', () => { test('returns undefined when the query errors', async () => { const errorResponse = { + headers: new Headers(), + body: '', status: 200, errors: [{message: 'Theme not found'} as any], } @@ -887,6 +891,8 @@ function themeAccessDeniedError(requiredAccess?: string, field = 'themes'): Clie return new ClientError( { + headers: new Headers(), + body: '', status: 200, errors: [ { diff --git a/packages/cli/package.json b/packages/cli/package.json index 2fc8b0116fd..dcd1b3e012b 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -55,7 +55,8 @@ "@ast-grep/napi": "0.43.0", "clipboardy": "4.0.0", "esbuild": "0.28.1", - "global-agent": "3.0.0" + "global-agent": "3.0.0", + "undici": "7.28.0" }, "devDependencies": { "@oclif/core": "4.8.3", diff --git a/packages/cli/src/bootstrap.ts b/packages/cli/src/bootstrap.ts index 0eddb65e982..519499b1ecd 100644 --- a/packages/cli/src/bootstrap.ts +++ b/packages/cli/src/bootstrap.ts @@ -7,6 +7,7 @@ */ import {loadCommand} from './command-registry.js' import {renderUncaughtError} from './uncaught-error-handler.js' +import {withFetchProxy} from './proxy.js' import {createGlobalProxyAgent} from 'global-agent' import {runCLI} from '@shopify/cli-kit/node/cli' @@ -47,11 +48,13 @@ interface RunShopifyCLIOptions { } async function runShopifyCLI({development}: RunShopifyCLIOptions) { - await runCLI({ - moduleURL: import.meta.url, - development, - lazyCommandLoader: loadCommand, - }) + await withFetchProxy(() => + runCLI({ + moduleURL: import.meta.url, + development, + lazyCommandLoader: loadCommand, + }), + ) } export default runShopifyCLI diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index a7a087bd4ce..9365fde852d 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -16,6 +16,7 @@ import DocSearch from './cli/commands/doc/search.js' import DocsGenerate from './cli/commands/docs/generate.js' import HelpCommand from './cli/commands/help.js' import {renderUncaughtError} from './uncaught-error-handler.js' +import {withFetchProxy} from './proxy.js' import List from './cli/commands/notifications/list.js' import Generate from './cli/commands/notifications/generate.js' import ClearCache from './cli/commands/cache/clear.js' @@ -78,10 +79,12 @@ interface RunShopifyCLIOptions { } async function runShopifyCLI({development}: RunShopifyCLIOptions) { - await runCLI({ - moduleURL: import.meta.url, - development, - }) + await withFetchProxy(() => + runCLI({ + moduleURL: import.meta.url, + development, + }), + ) } // Hide plugins command diff --git a/packages/cli/src/proxy-agent.ts b/packages/cli/src/proxy-agent.ts new file mode 100644 index 00000000000..7c655a68ae2 --- /dev/null +++ b/packages/cli/src/proxy-agent.ts @@ -0,0 +1,19 @@ +import {EnvHttpProxyAgent, getGlobalDispatcher, setGlobalDispatcher} from 'undici' + +export async function withProxyAgent(run: () => Promise, env: NodeJS.ProcessEnv): Promise { + const previousDispatcher = getGlobalDispatcher() + const dispatcher = new EnvHttpProxyAgent({ + // Empty values prevent Undici from using unprefixed proxy variables. + httpProxy: env.SHOPIFY_HTTP_PROXY ?? '', + httpsProxy: env.SHOPIFY_HTTPS_PROXY ?? '', + noProxy: env.SHOPIFY_NO_PROXY ?? '', + connectTimeout: 60000, + }) + setGlobalDispatcher(dispatcher) + try { + return await run() + } finally { + setGlobalDispatcher(previousDispatcher) + await dispatcher.close() + } +} diff --git a/packages/cli/src/proxy.test.ts b/packages/cli/src/proxy.test.ts new file mode 100644 index 00000000000..4903ad8e3a3 --- /dev/null +++ b/packages/cli/src/proxy.test.ts @@ -0,0 +1,206 @@ +import {withFetchProxy} from './proxy.js' +import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' +import {afterEach, expect, test, vi} from 'vitest' +import {getGlobalDispatcher} from 'undici' +import {createServer, type Server} from 'node:http' +import type {Socket} from 'node:net' + +afterEach(() => { + vi.unstubAllEnvs() +}) + +test('sends GraphQL requests through SHOPIFY_HTTP_PROXY and restores the dispatcher', async () => { + vi.stubEnv('NO_PROXY', '*') + vi.stubEnv('no_proxy', '*') + const previousDispatcher = getGlobalDispatcher() + const connections: string[] = [] + const proxy = createServer() + const sockets = trackSockets(proxy) + proxy.on('connect', (request, socket) => { + connections.push(request.url!) + socket.write('HTTP/1.1 200 Connection Established\r\n\r\n') + socket.once('data', () => { + const body = JSON.stringify({data: {ok: true}}) + socket.end( + `HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n${body}`, + ) + }) + }) + + try { + const proxyUrl = await listen(proxy) + const result = await withFetchProxy( + () => graphqlRequest({api: 'test', url: 'http://proxy-only.invalid/graphql', query: 'query { ok }'}), + {SHOPIFY_HTTP_PROXY: proxyUrl}, + ) + expect(result).toEqual({ok: true}) + expect(connections).toEqual(['proxy-only.invalid:80']) + expect(getGlobalDispatcher()).toBe(previousDispatcher) + } finally { + await close(proxy, sockets) + } +}) + +test('retries a native fetch socket failure and preserves GraphQL variables', async () => { + let requests = 0 + let body = '' + const server = createServer((request, response) => { + requests++ + if (requests === 1) { + request.socket.destroy() + return + } + request.setEncoding('utf8') + request.on('data', (chunk) => { + body += chunk + }) + request.on('end', () => { + response.writeHead(200, {'Content-Type': 'application/json'}) + response.end(JSON.stringify({data: {ok: true}})) + }) + }) + const sockets = trackSockets(server) + + try { + const url = await listen(server) + const result = await graphqlRequest({ + api: 'test', + url, + query: 'query Example($id: ID!) { node(id: $id) { id } }', + variables: {id: 'test-id'}, + preferredBehaviour: {useNetworkLevelRetry: true, maxRetryTimeMs: 2000, useAbortSignal: true, timeoutMs: 1000}, + }) + expect(result).toEqual({ok: true}) + expect(requests).toBe(2) + expect(JSON.parse(body)).toMatchObject({operationName: 'Example', variables: {id: 'test-id'}}) + } finally { + await close(server, sockets) + } +}) + +test('cancels an active native GraphQL request', async () => { + const controller = new AbortController() + const server = createServer(() => controller.abort()) + const sockets = trackSockets(server) + + try { + const url = await listen(server) + await expect( + graphqlRequest({ + api: 'test', + url, + query: 'query { ok }', + preferredBehaviour: {useNetworkLevelRetry: false, useAbortSignal: controller.signal}, + }), + ).rejects.toMatchObject({name: 'AbortError'}) + } finally { + await close(server, sockets) + } +}) + +test.each(['http', 'https'] as const)('uses the %s proxy for HTTPS requests', async (proxyVariable) => { + const connections: string[] = [] + const proxy = createServer() + const sockets = trackSockets(proxy) + proxy.on('connect', (request, socket) => { + connections.push(request.url!) + socket.end('HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\nConnection: close\r\n\r\n') + }) + + try { + const proxyUrl = await listen(proxy) + const env = + proxyVariable === 'http' + ? {SHOPIFY_HTTP_PROXY: proxyUrl} + : {SHOPIFY_HTTP_PROXY: 'http://127.0.0.1:1', SHOPIFY_HTTPS_PROXY: proxyUrl} + await expect( + withFetchProxy(() => fetch('https://proxy-only.invalid/graphql', {signal: AbortSignal.timeout(1000)}), env), + ).rejects.toThrow() + expect(connections).toEqual(['proxy-only.invalid:443']) + } finally { + await close(proxy, sockets) + } +}) + +test.each(['127.0.0.1', '*'])('bypasses the proxy with SHOPIFY_NO_PROXY=%s', async (noProxy) => { + const server = createServer((_request, response) => { + response.end('direct') + }) + const sockets = trackSockets(server) + + try { + const url = await listen(server) + const result = await withFetchProxy(async () => (await fetch(url)).text(), { + SHOPIFY_HTTP_PROXY: 'http://127.0.0.1:1', + SHOPIFY_NO_PROXY: noProxy, + }) + expect(result).toBe('direct') + } finally { + await close(server, sockets) + } +}) + +test('does not use unprefixed HTTP_PROXY when only SHOPIFY_HTTPS_PROXY is set', async () => { + vi.stubEnv('HTTP_PROXY', 'http://127.0.0.1:1') + vi.stubEnv('http_proxy', 'http://127.0.0.1:1') + const server = createServer((_request, response) => { + response.end('direct') + }) + const sockets = trackSockets(server) + + try { + const url = await listen(server) + const result = await withFetchProxy(async () => (await fetch(url)).text(), { + SHOPIFY_HTTPS_PROXY: 'http://127.0.0.1:1', + }) + expect(result).toBe('direct') + } finally { + await close(server, sockets) + } +}) + +test('leaves the dispatcher unchanged when no Shopify proxy is set', async () => { + const dispatcher = getGlobalDispatcher() + await withFetchProxy(async () => { + expect(getGlobalDispatcher()).toBe(dispatcher) + }, {}) + expect(getGlobalDispatcher()).toBe(dispatcher) +}) + +test('restores the dispatcher when the command fails', async () => { + const dispatcher = getGlobalDispatcher() + const error = new Error('command failed') + await expect( + withFetchProxy( + async () => { + throw error + }, + {SHOPIFY_HTTP_PROXY: 'http://127.0.0.1:1'}, + ), + ).rejects.toBe(error) + expect(getGlobalDispatcher()).toBe(dispatcher) +}) + +async function listen(server: Server): Promise { + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(0, '127.0.0.1', resolve) + }) + const address = server.address() + if (!address || typeof address === 'string') throw new Error('Expected a TCP address') + return `http://127.0.0.1:${address.port}` +} + +function trackSockets(server: Server): Set { + const sockets = new Set() + server.on('connection', (socket) => { + sockets.add(socket) + socket.once('close', () => sockets.delete(socket)) + }) + return sockets +} + +async function close(server: Server, sockets: Set): Promise { + for (const socket of sockets) socket.destroy() + await new Promise((resolve, reject) => server.close((error) => (error ? reject(error) : resolve()))) +} diff --git a/packages/cli/src/proxy.ts b/packages/cli/src/proxy.ts new file mode 100644 index 00000000000..015ebb4cc78 --- /dev/null +++ b/packages/cli/src/proxy.ts @@ -0,0 +1,6 @@ +export async function withFetchProxy(run: () => Promise, env: NodeJS.ProcessEnv = process.env): Promise { + if (!env.SHOPIFY_HTTP_PROXY && !env.SHOPIFY_HTTPS_PROXY) return run() + + const {withProxyAgent} = await import('./proxy-agent.js') + return withProxyAgent(run, env) +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a1f983b4f9..5dfce369c5f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -207,8 +207,8 @@ importers: specifier: 0.28.1 version: 0.28.1 graphql-request: - specifier: 6.1.0 - version: 6.1.0(graphql@16.14.2) + specifier: 7.4.0 + version: 7.4.0(graphql@16.14.2) h3: specifier: 1.15.11 version: 1.15.11 @@ -270,6 +270,9 @@ importers: global-agent: specifier: 3.0.0 version: 3.0.0 + undici: + specifier: 7.28.0 + version: 7.28.0 devDependencies: '@oclif/core': specifier: 4.8.3 @@ -410,8 +413,8 @@ importers: specifier: 16.14.2 version: 16.14.2 graphql-request: - specifier: 6.1.0 - version: 6.1.0(graphql@16.14.2) + specifier: 7.4.0 + version: 7.4.0(graphql@16.14.2) h3: specifier: 1.15.11 version: 1.15.11 @@ -5970,11 +5973,6 @@ packages: cosmiconfig-toml-loader: optional: true - graphql-request@6.1.0: - resolution: {integrity: sha512-p+XPfS4q7aIpKVcgmnZKhMNqhltk20hfXtkaIkTfjjmiKMJ5xrt5c743cL03y/K7y1rg3WrIC49xGiEQ4mxdNw==, tarball: https://registry.npmjs.org/graphql-request/-/graphql-request-6.1.0.tgz} - peerDependencies: - graphql: 16.14.2 - graphql-request@7.4.0: resolution: {integrity: sha512-xfr+zFb/QYbs4l4ty0dltqiXIp07U6sl+tOKAb0t50/EnQek6CVVBLjETXi+FghElytvgaAWtIOt3EV7zLzIAQ==, tarball: https://registry.npmjs.org/graphql-request/-/graphql-request-7.4.0.tgz} peerDependencies: @@ -15223,14 +15221,6 @@ snapshots: - utf-8-validate optional: true - graphql-request@6.1.0(graphql@16.14.2): - dependencies: - '@graphql-typed-document-node/core': 3.2.0(graphql@16.14.2) - cross-fetch: 3.2.0 - graphql: 16.14.2 - transitivePeerDependencies: - - encoding - graphql-request@7.4.0(graphql@16.14.2): dependencies: '@graphql-typed-document-node/core': 3.2.0(graphql@16.14.2) @@ -18000,8 +17990,7 @@ snapshots: dependencies: '@fastify/busboy': 2.1.1 - undici@7.28.0: - optional: true + undici@7.28.0: {} unicode-canonical-property-names-ecmascript@2.0.1: {}