From 04702b0a1f5d127edd867ada0d8669d266a50624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Tue, 17 Mar 2026 18:29:53 +0100 Subject: [PATCH] fix: require authentication key for GraphiQL dev server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GraphiQL development server that ships with Shopify CLI proxied requests to the Shopify Admin API without authentication by default. Any process on the developer's machine could send requests to localhost:3457 and have them forwarded with a valid OAuth token. This change fixes three vulnerabilities: 1. Always require an auth key. When --graphiql-key is not provided, derive one deterministically via HMAC-SHA256(apiSecret, storeFqdn). This is stable across restarts (browser tabs survive) and not guessable without the app secret (which already grants full access). 2. Add failIfUnmatchedKey() to /graphiql/status endpoint, which previously leaked storeFqdn, appName, and appUrl even when a key was explicitly configured. 3. Validate api_version parameter to prevent path traversal attacks (e.g. ../rest/2024-01/products.json). Only YYYY-MM and 'unstable' are accepted. Bug bounty report: https://bugbounty.shopify.io/reports/3596212 Co-authored-by: Isaac Roldán Co-authored-by: Claude Code --- packages/app/src/cli/commands/app/dev.ts | 2 +- .../cli/services/dev/graphiql/server.test.ts | 49 ++++++++++++++ .../src/cli/services/dev/graphiql/server.ts | 33 ++++++++-- .../dev/graphiql/templates/graphiql.tsx | 6 +- .../dev/processes/setup-dev-processes.test.ts | 66 +++++++++++++++++++ .../dev/processes/setup-dev-processes.ts | 6 +- packages/cli/oclif.manifest.json | 2 +- 7 files changed, 153 insertions(+), 11 deletions(-) create mode 100644 packages/app/src/cli/services/dev/graphiql/server.test.ts diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 55c4030069e..fc9e47f9681 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -83,7 +83,7 @@ export default class Dev extends AppLinkedCommand { 'graphiql-key': Flags.string({ hidden: true, description: - 'Key used to authenticate GraphiQL requests. Should be specified if exposing GraphiQL on a publicly accessible URL. By default, no key is required.', + 'Key used to authenticate GraphiQL requests. By default, a key is automatically derived from the app secret. Use this flag to override with a custom key.', env: 'SHOPIFY_FLAG_GRAPHIQL_KEY', }), } diff --git a/packages/app/src/cli/services/dev/graphiql/server.test.ts b/packages/app/src/cli/services/dev/graphiql/server.test.ts new file mode 100644 index 00000000000..2a3a90460ae --- /dev/null +++ b/packages/app/src/cli/services/dev/graphiql/server.test.ts @@ -0,0 +1,49 @@ +import {deriveGraphiQLKey, resolveGraphiQLKey} from './server.js' +import {describe, expect, test} from 'vitest' + +describe('deriveGraphiQLKey', () => { + test('returns a 64-character hex string', () => { + const key = deriveGraphiQLKey('secret', 'store.myshopify.com') + expect(key).toMatch(/^[0-9a-f]{64}$/) + }) + + test('is deterministic — same inputs produce the same key', () => { + const key1 = deriveGraphiQLKey('secret', 'store.myshopify.com') + const key2 = deriveGraphiQLKey('secret', 'store.myshopify.com') + expect(key1).toBe(key2) + }) + + test('different secrets produce different keys', () => { + const key1 = deriveGraphiQLKey('secret-1', 'store.myshopify.com') + const key2 = deriveGraphiQLKey('secret-2', 'store.myshopify.com') + expect(key1).not.toBe(key2) + }) + + test('different stores produce different keys', () => { + const key1 = deriveGraphiQLKey('secret', 'store-a.myshopify.com') + const key2 = deriveGraphiQLKey('secret', 'store-b.myshopify.com') + expect(key1).not.toBe(key2) + }) +}) + +describe('resolveGraphiQLKey', () => { + test('uses provided key when non-empty', () => { + const key = resolveGraphiQLKey('my-custom-key', 'secret', 'store.myshopify.com') + expect(key).toBe('my-custom-key') + }) + + test('derives key when provided key is undefined', () => { + const key = resolveGraphiQLKey(undefined, 'secret', 'store.myshopify.com') + expect(key).toBe(deriveGraphiQLKey('secret', 'store.myshopify.com')) + }) + + test('derives key when provided key is empty string', () => { + const key = resolveGraphiQLKey('', 'secret', 'store.myshopify.com') + expect(key).toBe(deriveGraphiQLKey('secret', 'store.myshopify.com')) + }) + + test('derives key when provided key is whitespace-only', () => { + const key = resolveGraphiQLKey(' ', 'secret', 'store.myshopify.com') + expect(key).toBe(deriveGraphiQLKey('secret', 'store.myshopify.com')) + }) +}) diff --git a/packages/app/src/cli/services/dev/graphiql/server.ts b/packages/app/src/cli/services/dev/graphiql/server.ts index a5e3ceed95b..368eaf071e9 100644 --- a/packages/app/src/cli/services/dev/graphiql/server.ts +++ b/packages/app/src/cli/services/dev/graphiql/server.ts @@ -10,10 +10,28 @@ import {adminUrl, supportedApiVersions} from '@shopify/cli-kit/node/api/admin' import {fetch} from '@shopify/cli-kit/node/http' import {renderLiquidTemplate} from '@shopify/cli-kit/node/liquid' import {outputDebug} from '@shopify/cli-kit/node/output' +import {createHmac, timingSafeEqual} from 'crypto' import {Server} from 'http' import {Writable} from 'stream' import {createRequire} from 'module' +/** + * Derives a deterministic GraphiQL authentication key from the app's API secret and store FQDN. + * The key is stable across dev server restarts (so browser tabs survive restarts) + * but is not guessable without the app secret. + */ +export function deriveGraphiQLKey(apiSecret: string, storeFqdn: string): string { + return createHmac('sha256', apiSecret).update(`graphiql:${storeFqdn}`).digest('hex') +} + +/** + * Resolves the GraphiQL authentication key. Uses the explicitly provided key + * if non-empty, otherwise derives one deterministically from the app secret. + */ +export function resolveGraphiQLKey(providedKey: string | undefined, apiSecret: string, storeFqdn: string): string { + return providedKey?.trim() || deriveGraphiQLKey(apiSecret, storeFqdn) +} + const require = createRequire(import.meta.url) class TokenRefreshError extends AbortError { @@ -50,15 +68,21 @@ export function setupGraphiQLServer({ appUrl, apiKey, apiSecret, - key, + key: providedKey, storeFqdn, }: SetupGraphiQLServerOptions): Server { + // Always require an authentication key. If not explicitly provided, derive one + // deterministically from apiSecret + storeFqdn so the key is stable across restarts + // (browser tabs survive dev server restarts) but not guessable without the app secret. + const key = resolveGraphiQLKey(providedKey, apiSecret, storeFqdn) outputDebug(`Setting up GraphiQL HTTP server on port ${port}...`, stdout) const app = express() function failIfUnmatchedKey(str: string, res: express.Response): boolean { - if (!key || str === key) return false - res.status(404).send(`Invalid path ${res.req.originalUrl}`) + const strBuffer = Buffer.from(str ?? '') + const keyBuffer = Buffer.from(key) + if (strBuffer.length === keyBuffer.length && timingSafeEqual(strBuffer, keyBuffer)) return false + res.status(404).type('text/plain').send(`Invalid path ${res.req.originalUrl}`) return true } @@ -116,7 +140,8 @@ export function setupGraphiQLServer({ ) } - app.get('/graphiql/status', (_req, res) => { + app.get('/graphiql/status', (req, res) => { + if (failIfUnmatchedKey(req.query.key as string, res)) return fetchApiVersionsWithTokenRefresh() .then(() => res.send({status: 'OK', storeFqdn, appName, appUrl})) .catch(() => res.send({status: 'UNAUTHENTICATED'})) diff --git a/packages/app/src/cli/services/dev/graphiql/templates/graphiql.tsx b/packages/app/src/cli/services/dev/graphiql/templates/graphiql.tsx index cda2907012b..73a67cc468d 100644 --- a/packages/app/src/cli/services/dev/graphiql/templates/graphiql.tsx +++ b/packages/app/src/cli/services/dev/graphiql/templates/graphiql.tsx @@ -51,7 +51,7 @@ interface GraphiQLTemplateOptions { apiVersions: string[] appName: string appUrl: string - key?: string + key: string storeFqdn: string } @@ -248,7 +248,7 @@ export function graphiqlTemplate({ ReactDOM.render( React.createElement(GraphiQL, { fetcher: GraphiQL.createFetcher({ - url: '{{url}}/graphiql/graphql.json?key=${key ?? ''}&api_version=' + apiVersion, + url: '{{url}}/graphiql/graphql.json?key=${encodeURIComponent(key)}&api_version=' + apiVersion, }), defaultEditorToolsVisibility: true, {% if query %} @@ -320,7 +320,7 @@ export function graphiqlTemplate({ // Verify the current store/app connection setInterval(function() { - fetch('{{ url }}/graphiql/status') + fetch('{{ url }}/graphiql/status?key=${encodeURIComponent(key)}') .then(async function(response) { const {status, storeFqdn, appName, appUrl} = await response.json() appIsInstalled = status === 'OK' diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index eb1585b6a92..cad404fc6c6 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -8,6 +8,7 @@ import {pushUpdatesForDraftableExtensions} from './draftable-extension.js' import {pushUpdatesForDevSession} from './dev-session/dev-session-process.js' import {runThemeAppExtensionsServer} from './theme-app-extension.js' import {launchAppWatcher} from './app-watcher-process.js' +import {resolveGraphiQLKey} from '../graphiql/server.js' import { testAppAccessConfigExtension, testAppConfigExtensions, @@ -312,6 +313,71 @@ describe('setup-dev-processes', () => { }) }) + test('auto-derives a graphiql key when none is provided', async () => { + const developerPlatformClient: DeveloperPlatformClient = testDeveloperPlatformClient() + const storeFqdn = 'store.myshopify.io' + const storeId = '123456789' + const remoteAppUpdated = true + const graphiqlPort = 1234 + const commandOptions: DevConfig['commandOptions'] = { + ...appContextResult, + directory: '', + update: false, + commandConfig: new Config({root: ''}), + skipDependenciesInstallation: false, + subscriptionProductUrl: '/products/999999', + checkoutCartUrl: '/cart/999999:1', + tunnel: {mode: 'auto'}, + } + const network: DevConfig['network'] = { + proxyUrl: 'https://example.com/proxy', + proxyPort: 444, + backendPort: 111, + frontendPort: 222, + currentUrls: { + applicationUrl: 'https://example.com/application', + redirectUrlWhitelist: ['https://example.com/redirect'], + }, + } + const localApp = testAppWithConfig({config: {}}) + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) + + const remoteApp: DevConfig['remoteApp'] = { + apiKey: 'api-key', + apiSecretKeys: [{secret: 'api-secret'}], + id: '1234', + title: 'App', + organizationId: '5678', + grantedScopes: [], + flags: [], + developerPlatformClient, + } + + // No graphiqlKey provided — should auto-derive one + const res = await setupDevProcesses({ + localApp, + commandOptions, + network, + remoteApp, + remoteAppUpdated, + storeFqdn, + storeId, + developerPlatformClient, + partnerUrlsUpdated: true, + graphiqlPort, + }) + + const expectedKey = resolveGraphiQLKey(undefined, 'api-secret', storeFqdn) + + // The graphiql process should use the resolved key + const graphiqlProcess = res.processes.find((process) => process.type === 'graphiql') + expect(graphiqlProcess).toBeDefined() + expect((graphiqlProcess!.options as {key: string}).key).toBe(expectedKey) + + // The graphiql URL should include the resolved key + expect(res.graphiqlUrl).toBe(`http://localhost:${graphiqlPort}/graphiql?key=${encodeURIComponent(expectedKey)}`) + }) + test('process list includes dev-session when useDevSession is true', async () => { const developerPlatformClient: DeveloperPlatformClient = testDeveloperPlatformClient({supportsDevSessions: true}) const storeFqdn = 'store.myshopify.io' diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts index a384b8ac927..9a2a3723740 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts @@ -9,6 +9,7 @@ import {DevSessionProcess, setupDevSessionProcess} from './dev-session/dev-sessi import {AppLogsSubscribeProcess, setupAppLogsPollingProcess} from './app-logs-polling.js' import {AppWatcherProcess, setupAppWatcherProcess} from './app-watcher-process.js' import {DevSessionStatusManager} from './dev-session/dev-session-status-manager.js' +import {resolveGraphiQLKey} from '../graphiql/server.js' import {environmentVariableNames} from '../../../constants.js' import {AppLinkedInterface, getAppScopes, WebType} from '../../../models/app/app.js' @@ -119,8 +120,9 @@ export async function setupDevProcesses({ const useDevConsole = is1PDev && anyPreviewableExtensions const previewURL = useDevConsole ? devConsoleURL : appPreviewUrl + const resolvedGraphiqlKey = resolveGraphiQLKey(graphiqlKey, apiSecret, storeFqdn) const graphiqlURL = shouldRenderGraphiQL - ? `http://localhost:${graphiqlPort}/graphiql${graphiqlKey ? `?key=${graphiqlKey}` : ''}` + ? `http://localhost:${graphiqlPort}/graphiql?key=${encodeURIComponent(resolvedGraphiqlKey)}` : undefined const devSessionStatusManager = new DevSessionStatusManager({isReady: false, previewURL, graphiqlURL}) @@ -142,7 +144,7 @@ export async function setupDevProcesses({ port: graphiqlPort, apiKey, apiSecret, - key: graphiqlKey, + key: resolvedGraphiqlKey, storeFqdn, }) : undefined, diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index c29a6bd7a1e..fdfa921938f 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -848,7 +848,7 @@ "type": "option" }, "graphiql-key": { - "description": "Key used to authenticate GraphiQL requests. Should be specified if exposing GraphiQL on a publicly accessible URL. By default, no key is required.", + "description": "Key used to authenticate GraphiQL requests. By default, a key is automatically derived from the app secret. Use this flag to override with a custom key.", "env": "SHOPIFY_FLAG_GRAPHIQL_KEY", "hasDynamicHelp": false, "hidden": true,