diff --git a/.changeset/keypress-accepted-keys.md b/.changeset/keypress-accepted-keys.md new file mode 100644 index 00000000000..aef7cdc7589 --- /dev/null +++ b/.changeset/keypress-accepted-keys.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +`keypress` accepts a `keys` option: only those keys end the wait, and other keys are ignored. `keypress` now sets raw mode back to its earlier state, and it also detects Ctrl+C when an Ink renderer has set the stdin encoding. diff --git a/.changeset/theme-trust-challenge.md b/.changeset/theme-trust-challenge.md new file mode 100644 index 00000000000..1854acedd66 --- /dev/null +++ b/.changeset/theme-trust-challenge.md @@ -0,0 +1,7 @@ +--- +'@shopify/cli-kit': minor +'@shopify/theme': patch +'@shopify/app': patch +--- + +Handle Trust Battery identity challenges in theme commands. When Shopify asks you to verify your identity, the CLI opens the verification page in your browser and continues after you complete it. In CI, with `--no-input`, or with `--json`, the command stops and prints the verification link. `app dev` stops creating its host theme when you cancel the verification or it fails, instead of trying again. diff --git a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts index 2524a1db019..cbb8ed20cc7 100644 --- a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts +++ b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts @@ -1,6 +1,7 @@ import {waitForThemeToBeProcessed} from './host-theme-watcher.js' import {HostThemeManager, DEFAULT_THEME_ZIP, FALLBACK_THEME_ZIP} from './host-theme-manager.js' -import {themeCreate} from '@shopify/cli-kit/node/themes/api' +import {isTrustChallengeError, themeCreate} from '@shopify/cli-kit/node/themes/api' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' import {beforeEach, describe, expect, test, vi} from 'vitest' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' import {AdminSession} from '@shopify/cli-kit/node/session' @@ -179,5 +180,45 @@ describe('HostThemeManager', () => { ) expect(themeCreate).toHaveBeenCalledTimes(4) }) + + test('stops retrying when the Trust Battery prompt is cancelled with Ctrl+C', async () => { + // Given + vi.mocked(themeCreate).mockRejectedValue(new AbortSilentError()) + + // When + // Then + await expect(themeManager.findOrCreate()).rejects.toBeInstanceOf(AbortSilentError) + expect(themeCreate).toHaveBeenCalledTimes(1) + }) + + test('stops retrying when a Trust Battery challenge ends the request', async () => { + // Given + const challengeError = new AbortError('Shopify still needs to verify your identity before it can continue.') + vi.mocked(themeCreate).mockRejectedValue(challengeError) + vi.mocked(isTrustChallengeError).mockImplementation((error) => error === challengeError) + + // When + // Then + await expect(themeManager.findOrCreate()).rejects.toBe(challengeError) + expect(themeCreate).toHaveBeenCalledTimes(1) + }) + + test('keeps retrying a theme creation abort that is not a Trust Battery challenge', async () => { + // Given + vi.mocked(themeCreate).mockRejectedValueOnce(new AbortError('Source is empty')).mockResolvedValueOnce({ + id: 12345, + name: 'Theme', + role: 'development', + createdAtRuntime: true, + processing: true, + }) + vi.mocked(isTrustChallengeError).mockReturnValue(false) + + // When + await themeManager.findOrCreate() + + // Then + expect(themeCreate).toHaveBeenCalledTimes(2) + }) }) }) diff --git a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts index c80d08ee1d4..569da4c5d46 100644 --- a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts +++ b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts @@ -3,9 +3,9 @@ import {getHostTheme, removeHostTheme, setHostTheme} from '@shopify/cli-kit/node import {ThemeManager} from '@shopify/cli-kit/node/themes/theme-manager' import {AdminSession} from '@shopify/cli-kit/node/session' import {Theme} from '@shopify/cli-kit/node/themes/types' -import {themeCreate} from '@shopify/cli-kit/node/themes/api' +import {isTrustChallengeError, themeCreate} from '@shopify/cli-kit/node/themes/api' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' -import {BugError} from '@shopify/cli-kit/node/error' +import {AbortSilentError, BugError} from '@shopify/cli-kit/node/error' import {outputDebug} from '@shopify/cli-kit/node/output' export const DEFAULT_THEME_ZIP = 'https://codeload.github.com/Shopify/dawn/zip/refs/tags/v15.0.0' @@ -60,8 +60,8 @@ export class HostThemeManager extends ThemeManager { } else { throw new Error() } - // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { + if (error instanceof AbortSilentError || isTrustChallengeError(error)) throw error outputDebug(`Failed to create theme with name "${options.name}" and role "${options.role}". Retrying...`) } } diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts new file mode 100644 index 00000000000..518c8507c8d --- /dev/null +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -0,0 +1,357 @@ +import {TrustChallengeError, trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' +import {GraphQLClientError} from '../api/headers.js' +import {commandEventOutputMode} from '../command-event-context.js' +import {recordEvent} from '../../../public/node/analytics.js' +import {adminFqdn} from '../../../public/node/context/fqdn.js' +import {AbortError, AbortSilentError} from '../../../public/node/error.js' +import {isCI, openURL} from '../../../public/node/system.js' +import {isTTY, keypress, renderInfo, renderSuccess} from '../../../public/node/ui.js' +import {beforeEach, describe, expect, test, vi} from 'vitest' +import {ClientError} from 'graphql-request' + +vi.mock('../command-event-context.js') +vi.mock('../../../public/node/analytics.js') +vi.mock('../../../public/node/context/fqdn.js') +vi.mock('../../../public/node/output.js') +vi.mock('../../../public/node/system.js') +vi.mock('../../../public/node/ui.js') + +const challengeUrl = 'https://admin.shopify.com/challenges/user_verification?token=abc' + +beforeEach(() => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') + vi.mocked(commandEventOutputMode).mockReturnValue(undefined) + vi.mocked(isCI).mockReturnValue(false) + vi.mocked(isTTY).mockReturnValue(true) + vi.mocked(openURL).mockResolvedValue(true) + vi.mocked(keypress).mockResolvedValue(undefined) +}) + +describe('trustChallengeUrl', () => { + test('returns the redirect URL from a CHALLENGE_REQUIRED graphql-request ClientError', async () => { + const error = challengeClientError(challengeUrl) + + await expect(trustChallengeUrl(error)).resolves.toBe(challengeUrl) + }) + + test('returns the redirect URL from a CHALLENGE_REQUIRED cli-kit GraphQLClientError', async () => { + const error = new GraphQLClientError('Challenge Required', 200, [ + {message: 'Challenge Required', extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: challengeUrl}}, + ]) + + await expect(trustChallengeUrl(error)).resolves.toBe(challengeUrl) + }) + + test('returns undefined for a GraphQL error with a different code', async () => { + const error = new ClientError( + {status: 200, errors: [{message: 'Access denied', extensions: {code: 'ACCESS_DENIED'}} as any]}, + {query: ''}, + ) + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is not on the Admin host', async () => { + const error = challengeClientError('https://evil.example.com/challenges/user_verification?token=abc') + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is not https', async () => { + const error = challengeClientError('http://admin.shopify.com/challenges/user_verification?token=abc') + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is missing', async () => { + const error = new ClientError( + {status: 200, errors: [{message: 'Challenge Required', extensions: {code: 'CHALLENGE_REQUIRED'}} as any]}, + {query: ''}, + ) + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined for an error that is not a GraphQL client error', async () => { + await expect(trustChallengeUrl(new Error('boom'))).resolves.toBeUndefined() + }) + + test('accepts the local Admin host when the CLI targets a local environment', async () => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shop.dev') + const localChallengeUrl = 'https://admin.shop.dev/challenges/user_verification?token=abc' + + await expect(trustChallengeUrl(challengeClientError(localChallengeUrl))).resolves.toBe(localChallengeUrl) + }) +}) + +describe('withTrustChallengeRetry', () => { + test('returns the result of a request that succeeds without a challenge', async () => { + const request = vi.fn().mockResolvedValue('ok') + + await expect(withTrustChallengeRetry(request)).resolves.toBe('ok') + + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(keypress).not.toHaveBeenCalled() + }) + + test('rethrows an error that is not a trust challenge without prompting', async () => { + const error = new Error('boom') + const request = vi.fn().mockRejectedValue(error) + + await expect(withTrustChallengeRetry(request)).rejects.toBe(error) + + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + }) + + test('opens the challenge URL, waits for Enter, then retries the request once', async () => { + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') + + await expect(withTrustChallengeRetry(request)).resolves.toBe('retried') + + expect(request).toHaveBeenCalledTimes(2) + expect(openURL).toHaveBeenCalledWith(challengeUrl) + // Only Enter or Space answer the prompt: `theme dev` binds other keys, such as `t`, on the same stdin. + expect(keypress).toHaveBeenCalledExactlyOnceWith(process.stdin, undefined, {keys: ['\r', '\n', ' ']}) + expect(renderInfo).toHaveBeenCalledWith( + expect.objectContaining({ + headline: 'Verify your identity to continue.', + nextSteps: expect.arrayContaining([expect.arrayContaining([{userInput: 'Enter'}])]), + }), + ) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:required') + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-opened') + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:acknowledged') + expect(renderSuccess).toHaveBeenCalledWith({headline: 'Continuing.'}) + }) + + test('renders a link to the challenge URL when the browser cannot be opened', async () => { + vi.mocked(openURL).mockResolvedValue(false) + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') + + await withTrustChallengeRetry(request) + + expect(renderInfo).toHaveBeenCalledWith( + expect.objectContaining({link: {label: 'Open the verification page', url: challengeUrl}}), + ) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-not-opened') + }) + + test('aborts with the challenge URL when the retried request is challenged again', async () => { + const request = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(TrustChallengeError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(2) + expect(keypress).toHaveBeenCalledTimes(1) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:retry-challenged') + }) + + test('rethrows a non-challenge error from the retried request unchanged', async () => { + const retryError = new Error('boom') + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockRejectedValueOnce(retryError) + + await expect(withTrustChallengeRetry(request)).rejects.toBe(retryError) + + expect(request).toHaveBeenCalledTimes(2) + }) + + test('aborts with the challenge URL in the next steps when running in CI', async () => { + vi.mocked(isCI).mockReturnValue(true) + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(TrustChallengeError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') + }) + + test('aborts with the challenge URL instead of prompting in JSON output mode', async () => { + vi.mocked(commandEventOutputMode).mockReturnValue('json') + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(keypress).not.toHaveBeenCalled() + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') + }) + + test('aborts instead of prompting when stdin is not a TTY', async () => { + vi.mocked(isTTY).mockReturnValue(false) + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + await expect(withTrustChallengeRetry(request)).rejects.toBeInstanceOf(AbortError) + + expect(keypress).not.toHaveBeenCalled() + }) + + test('shares a single prompt across concurrent requests that are challenged together', async () => { + const enter = deferredEnter() + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('second') + + const results = Promise.all([withTrustChallengeRetry(firstRequest), withTrustChallengeRetry(secondRequest)]) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) + enter.press() + + await expect(results).resolves.toEqual(['first', 'second']) + expect(openURL).toHaveBeenCalledTimes(1) + expect(renderSuccess).toHaveBeenCalledTimes(1) + expect(firstRequest).toHaveBeenCalledTimes(2) + expect(secondRequest).toHaveBeenCalledTimes(2) + }) + + test('holds a request that starts while a challenge is pending until Enter is pressed, then sends it once', async () => { + const enter = deferredEnter() + const challengedRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('challenged') + const laterRequest = vi.fn().mockResolvedValue('later') + + const challengedResult = withTrustChallengeRetry(challengedRequest) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) + + const laterResult = withTrustChallengeRetry(laterRequest) + await Promise.resolve() + expect(laterRequest).not.toHaveBeenCalled() + + enter.press() + + await expect(Promise.all([challengedResult, laterResult])).resolves.toEqual(['challenged', 'later']) + expect(laterRequest).toHaveBeenCalledTimes(1) + expect(openURL).toHaveBeenCalledTimes(1) + }) + + test('retries without a second prompt when a request already in flight is challenged after the verification', async () => { + const enter = deferredEnter() + let rejectSecondResponse!: (error: unknown) => void + const secondResponse = new Promise((_resolve, reject) => { + rejectSecondResponse = reject + }) + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi.fn().mockReturnValueOnce(secondResponse).mockResolvedValueOnce('second') + + const firstResult = withTrustChallengeRetry(firstRequest) + const secondResult = withTrustChallengeRetry(secondRequest) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) + enter.press() + await expect(firstResult).resolves.toBe('first') + + rejectSecondResponse(challengeClientError(challengeUrl)) + + await expect(secondResult).resolves.toBe('second') + expect(secondRequest).toHaveBeenCalledTimes(2) + expect(keypress).toHaveBeenCalledTimes(1) + expect(openURL).toHaveBeenCalledTimes(1) + }) + + test('waits for a newer open prompt before retrying a request that was in flight during an earlier verification', async () => { + const firstEnter = Promise.withResolvers() + const secondEnter = Promise.withResolvers() + vi.mocked(keypress).mockReturnValueOnce(firstEnter.promise).mockReturnValueOnce(secondEnter.promise) + const secondResponse = Promise.withResolvers() + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi.fn().mockReturnValueOnce(secondResponse.promise).mockResolvedValueOnce('second') + const thirdRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('third') + + const firstResult = withTrustChallengeRetry(firstRequest) + const secondResult = withTrustChallengeRetry(secondRequest) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) + firstEnter.resolve() + await expect(firstResult).resolves.toBe('first') + + const thirdResult = withTrustChallengeRetry(thirdRequest) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(2)) + + secondResponse.reject(challengeClientError(challengeUrl)) + // No timer is involved: the mocks resolve at once, so one event-loop turn runs the + // second request's whole challenge check before the assertion. + await new Promise((resolve) => setImmediate(resolve)) + expect(secondRequest).toHaveBeenCalledTimes(1) + + secondEnter.resolve() + + await expect(Promise.all([secondResult, thirdResult])).resolves.toEqual(['second', 'third']) + expect(secondRequest).toHaveBeenCalledTimes(2) + expect(keypress).toHaveBeenCalledTimes(2) + }) + + test('rejects requests waiting on a pending challenge when the prompt is cancelled with Ctrl+C', async () => { + const enter = deferredEnter() + const challengedRequest = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)) + const laterRequest = vi.fn().mockResolvedValue('later') + + const challengedResult = withTrustChallengeRetry(challengedRequest) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) + const laterResult = withTrustChallengeRetry(laterRequest) + + enter.cancel() + + await expect(challengedResult).rejects.toBeInstanceOf(AbortSilentError) + await expect(laterResult).rejects.toBeInstanceOf(AbortSilentError) + expect(laterRequest).not.toHaveBeenCalled() + expect(renderSuccess).not.toHaveBeenCalled() + }) +}) + +function deferredEnter(): {press: () => void; cancel: () => void} { + let press!: () => void + let cancel!: () => void + vi.mocked(keypress).mockReturnValue( + new Promise((resolve, reject) => { + press = resolve + cancel = () => reject(new AbortSilentError()) + }), + ) + return {press: () => press(), cancel: () => cancel()} +} + +function challengeClientError(redirectTo: string): ClientError { + return new ClientError( + { + status: 200, + errors: [ + { + message: 'Challenge Required', + extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: redirectTo}, + } as any, + ], + }, + {query: ''}, + ) +} diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts new file mode 100644 index 00000000000..255457c5720 --- /dev/null +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -0,0 +1,157 @@ +import {extractGraphQLErrors} from '../api/graphql.js' +import {commandEventOutputMode} from '../command-event-context.js' +import {recordEvent} from '../../../public/node/analytics.js' +import {adminFqdn} from '../../../public/node/context/fqdn.js' +import {AbortError} from '../../../public/node/error.js' +import {outputDebug} from '../../../public/node/output.js' +import {isCI, openURL} from '../../../public/node/system.js' +import {isTTY, keypress, renderInfo, renderSuccess} from '../../../public/node/ui.js' + +const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' +// Enter or Space. `theme dev` binds other single keys on the same stdin to its own shortcuts. +const CONTINUE_KEYS = ['\r', '\n', ' '] + +/** + * The abort that ends a Trust Battery challenge: the session cannot answer it, or + * the request was challenged again after the verification. + */ +export class TrustChallengeError extends AbortError {} + +let pendingChallenge: Promise | undefined +let completedChallenges = 0 + +/** + * Runs a theme Admin API request and, when Core answers with a Trust Battery + * challenge, waits for the user to complete it in the browser before retrying once. + * + * A request that starts while another request's challenge is still being completed + * waits for that challenge first, so it is sent once instead of being challenged + * itself and then retried. A request that was already in flight when a challenge + * was completed retries without a second prompt, unless another prompt is open: + * then it waits for that prompt too. + * + * @param request - The request to run and possibly retry. + * @returns The result of the request. + */ +export async function withTrustChallengeRetry(request: () => Promise): Promise { + if (pendingChallenge) await pendingChallenge + const completedChallengesAtStart = completedChallenges + + const attempt = await attemptRequest(request) + if (!attempt.challenged) return attempt.result + + const verifiedSinceStart = completedChallenges > completedChallengesAtStart + if (pendingChallenge || !verifiedSinceStart) await completeTrustChallenge(attempt.challengeUrl) + + const retry = await attemptRequest(request) + if (!retry.challenged) return retry.result + + recordEvent('theme-api:trust-challenge:retry-challenged') + throw new TrustChallengeError('Shopify still needs to verify your identity before it can continue.', undefined, [ + ['Complete the verification at', {link: {url: retry.challengeUrl}}, 'then run the command again.'], + ]) +} + +type RequestAttempt = {challenged: false; result: T} | {challenged: true; challengeUrl: string} + +/** + * Runs the request once. A Trust Battery challenge becomes a result with the challenge URL; + * any other error is thrown. + */ +async function attemptRequest(request: () => Promise): Promise> { + try { + return {challenged: false, result: await request()} + } catch (error) { + const challengeUrl = await trustChallengeUrl(error) + if (!challengeUrl) throw error + return {challenged: true, challengeUrl} + } +} + +/** + * Extracts the challenge URL from a Trust Battery `CHALLENGE_REQUIRED` GraphQL error. + * + * Only URLs on the Admin host are trusted. Anything else is not treated as a + * challenge and the original error surfaces through normal error handling. + * + * @param error - The error thrown by an Admin API request. + * @returns The challenge URL, or undefined when the error is not a trust challenge. + */ +export async function trustChallengeUrl(error: unknown): Promise { + const challengeError = extractGraphQLErrors(error).find( + (graphQLError) => graphQLError.extensions?.code === TRUST_CHALLENGE_ERROR_CODE, + ) + const redirectTo = challengeError?.extensions?.redirect_to + if (typeof redirectTo !== 'string') return undefined + + if (!(await isAdminUrl(redirectTo))) { + outputDebug(`Ignoring trust challenge with an unexpected URL host: ${redirectTo}`) + return undefined + } + + return redirectTo +} + +async function isAdminUrl(url: string): Promise { + try { + const parsedUrl = new URL(url) + return parsedUrl.protocol === 'https:' && parsedUrl.hostname === (await adminFqdn()) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + +/** + * Concurrent theme requests (for example the parallel batches of `theme push`) + * all receive the same challenge. Share one prompt so the browser opens once and + * every waiting request retries after the same verification. + */ +async function completeTrustChallenge(challengeUrl: string): Promise { + pendingChallenge ??= promptForTrustChallenge(challengeUrl) + .then(() => { + completedChallenges += 1 + }) + .finally(() => { + pendingChallenge = undefined + }) + return pendingChallenge +} + +async function promptForTrustChallenge(challengeUrl: string): Promise { + recordEvent('theme-api:trust-challenge:required') + + if (isCI() || !isTTY() || commandEventOutputMode() === 'json') { + recordEvent('theme-api:trust-challenge:non-interactive') + throw new TrustChallengeError('Shopify needs to verify your identity before it can continue.', undefined, [ + [ + 'Open', + {link: {url: challengeUrl}}, + "in a browser where you're logged in to Shopify with the same account you use in Shopify CLI, complete the verification, then run the command again.", + ], + ]) + } + + const opened = await openURL(challengeUrl) + recordEvent(opened ? 'theme-api:trust-challenge:browser-opened' : 'theme-api:trust-challenge:browser-not-opened') + + // Everything the user needs is inside the banner: long-running commands such as + // `theme push` redraw an Ink progress bar that erases any plain line written after it. + renderInfo({ + headline: 'Verify your identity to continue.', + body: opened + ? 'A verification page opened in your browser.' + : 'Shopify needs to verify your identity before it can continue.', + link: opened ? undefined : {label: 'Open the verification page', url: challengeUrl}, + nextSteps: [ + ['Complete the verification in your browser, logged in with the same account you use in Shopify CLI.'], + ['Press', {userInput: 'Enter'}, 'here once you have completed it.'], + ], + orderedNextSteps: true, + }) + + await keypress(process.stdin, undefined, {keys: CONTINUE_KEYS}) + + recordEvent('theme-api:trust-challenge:acknowledged') + renderSuccess({headline: 'Continuing.'}) +} 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 abf10d832f0..4e2faaa8d65 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -31,11 +31,16 @@ import {MetafieldDefinitionsByOwnerType} from '../../../cli/api/graphql/admin/ge import {errorHandler} from '../../../private/node/api/graphql.js' import {adminRequestDoc, supportedApiVersions} from '../api/admin.js' import {AbortError} from '../error.js' +import {adminFqdn} from '../context/fqdn.js' +import {openURL} from '../system.js' +import {isTTY, keypress} from '../ui.js' import {test, vi, expect, describe, beforeEach} from 'vitest' import {ClientError} from 'graphql-request' vi.mock('../api/admin.js') +vi.mock('../context/fqdn.js') +vi.mock('../ui.js') vi.mock('@shopify/cli-kit/node/system') vi.stubGlobal('fetch', vi.fn()) @@ -562,6 +567,79 @@ describe('themePublish', () => { }) }) +describe('trust challenges', () => { + const challengeUrl = 'https://admin.shopify.com/challenges/user_verification?token=abc' + + beforeEach(() => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') + vi.mocked(isTTY).mockReturnValue(true) + vi.mocked(openURL).mockResolvedValue(true) + vi.mocked(keypress).mockResolvedValue(undefined) + }) + + test('themePublish opens the challenge in the browser and retries after the user completes it', async () => { + // Given + const id = 123 + vi.mocked(adminRequestDoc) + .mockRejectedValueOnce(trustChallengeError(challengeUrl)) + .mockResolvedValueOnce({ + themePublish: { + theme: {id: `gid://shopify/OnlineStoreTheme/${id}`, name: 'live theme', role: 'live'}, + userErrors: [], + }, + }) + + // When + const theme = await themePublish(id, session) + + // Then + expect(openURL).toHaveBeenCalledWith(challengeUrl) + expect(keypress).toHaveBeenCalledTimes(1) + expect(adminRequestDoc).toHaveBeenCalledTimes(2) + expect(theme!.id).toEqual(id) + }) + + test('bulkUploadThemeAssets retries the challenged batch after the user completes the challenge', async () => { + // Given + vi.mocked(adminRequestDoc) + .mockRejectedValueOnce(trustChallengeError(challengeUrl)) + .mockResolvedValueOnce({ + themeFilesUpsert: { + upsertedThemeFiles: [{filename: 'snippets/product-variant-picker.liquid'}], + userErrors: [], + }, + }) + + // When + const results = await bulkUploadThemeAssets( + 123, + [{key: 'snippets/product-variant-picker.liquid', value: 'content'}], + session, + ) + + // Then + expect(keypress).toHaveBeenCalledTimes(1) + expect(adminRequestDoc).toHaveBeenCalledTimes(2) + expect(results).toEqual([ + {key: 'snippets/product-variant-picker.liquid', success: true, operation: Operation.Upload}, + ]) + }) + + test('a non-challenge GraphQL error is not retried', async () => { + // Given + const error = new ClientError({status: 200, errors: [{message: 'boom'} as any]}, {query: ''}) + vi.mocked(adminRequestDoc).mockRejectedValue(error) + + // When + const promise = themePublish(123, session) + + // Then + await expect(promise).rejects.toBe(error) + expect(openURL).not.toHaveBeenCalled() + expect(adminRequestDoc).toHaveBeenCalledTimes(1) + }) +}) + describe('deleteThemeAssets', () => { test('deletes a theme asset', async () => { // Given @@ -940,6 +1018,21 @@ describe('parseThemeFileContent', () => { }) }) +function trustChallengeError(redirectTo: string): ClientError { + return new ClientError( + { + status: 200, + errors: [ + { + message: 'Challenge Required', + extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: redirectTo}, + } as any, + ], + }, + {query: ''}, + ) +} + function themeAccessDeniedError(requiredAccess?: string, field = 'themes'): ClientError { const extensions = requiredAccess ? {code: 'ACCESS_DENIED', requiredAccess} : {code: 'ACCESS_DENIED'} const message = requiredAccess diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index ddbbdbc831f..9ec1e8100b8 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -28,6 +28,7 @@ import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' import {OnlineStorePasswordProtection} from '../../../cli/api/graphql/admin/generated/online_store_password_protection.js' import {extractGraphQLErrors} from '../../../private/node/api/graphql.js' +import {TrustChallengeError, withTrustChallengeRetry} from '../../../private/node/themes/trust-challenge.js' import {RequestModeInput} from '../http.js' import {adminRequestDoc, type AdminRequestOptions} from '../api/admin.js' import {AdminSession} from '../session.js' @@ -625,11 +626,23 @@ function unexpectedGraphQLError(message: string): never { throw recordError(new AbortError(message)) } +/** + * Whether the error ends a Trust Battery challenge on a theme request. A caller that + * retries theme requests must rethrow it, or the user is asked to verify again for + * each attempt. + * + * @param error - The error to check. + * @returns True when the error ends a Trust Battery challenge. + */ +export function isTrustChallengeError(error: unknown): boolean { + return error instanceof TrustChallengeError +} + async function requestThemeAdminDoc( options: AdminRequestOptions, ): Promise { try { - const response = await adminRequestDoc(options) + const response = await withTrustChallengeRetry(() => adminRequestDoc(options)) return response } catch (error) { abortIfMissingThemeAccessScope(error) diff --git a/packages/cli-kit/src/public/node/ui.test.ts b/packages/cli-kit/src/public/node/ui.test.ts index dc299221e1c..d38962e4acf 100644 --- a/packages/cli-kit/src/public/node/ui.test.ts +++ b/packages/cli-kit/src/public/node/ui.test.ts @@ -14,7 +14,7 @@ import { Task, } from './ui.js' import {AbortSignal} from './abort.js' -import {BugError, FatalError, AbortError, FatalErrorType} from './error.js' +import {BugError, FatalError, AbortError, AbortSilentError} from './error.js' import {renderCommandEventAsJson, runWithCommandEvents, runWithCommandEventsForCommand} from './command-events.js' import {mockAndCaptureOutput, withCapturedStandardStreams} from './testing/output.js' import {outputResult, TokenizedString, unstyled} from './output.js' @@ -23,6 +23,8 @@ import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import supportsHyperlinks from 'supports-hyperlinks' import {Writable} from 'stream' +import {EventEmitter} from 'events' +import {setImmediate} from 'timers/promises' import type {RenderOptions} from 'ink' vi.mock('supports-hyperlinks') @@ -744,54 +746,131 @@ describe('renderTasks', async () => { }) }) +class FakeStdin extends EventEmitter { + isRaw: boolean + setRawMode = vi.fn((mode: boolean) => { + this.isRaw = mode + return this + }) + + ref = vi.fn(() => this) + unref = vi.fn(() => this) + + constructor({isRaw}: {isRaw: boolean}) { + super() + this.isRaw = isRaw + } + + press(key: string): void { + this.emit('data', Buffer.from(key)) + } + + // An active Ink renderer calls `setEncoding('utf8')`, and then stdin emits strings. + pressEncoded(key: string): void { + this.emit('data', key) + } + + asStdin(): typeof process.stdin { + return this as unknown as typeof process.stdin + } +} + describe('keypress', async () => { - test('waits for a keypress, managing stdin', async () => { - let registeredListener: any - const mockStdin = { - setRawMode: vi.fn(), - once: (event: string, callback: any) => { - registeredListener = callback - }, - ref: vi.fn(), - unref: vi.fn(), - } as any + const enterKeys = {keys: ['\r', '\n', ' ']} + + test('resolves on any key when no keys are given', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) - const promise = keypress(mockStdin, {skipTTYCheck: true}) - expect(mockStdin.ref).toBeCalled() - expect(mockStdin.setRawMode).toHaveBeenLastCalledWith(true) - // create a buffer representing pressing the enter key - registeredListener(Buffer.from([13])) + stdin.press('x') + + await expect(promise).resolves.toBeUndefined() + }) + + test.each([ + ['a carriage return', '\r'], + ['a line feed', '\n'], + ['a space', ' '], + ['a multi-character chunk that contains an accepted key', 'ab\r'], + ])('resolves on %s when it is an accepted key', async (_name, key) => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + stdin.press(key) + + await expect(promise).resolves.toBeUndefined() + }) + + test('ignores keys that are not accepted', async () => { + const stdin = new FakeStdin({isRaw: false}) + const settled = vi.fn() + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys).then(settled) + + stdin.press('t') + stdin.pressEncoded('p') + await setImmediate() + expect(settled).not.toHaveBeenCalled() + + stdin.press('\r') + + await expect(promise).resolves.toBeUndefined() + }) + test('resolves when an accepted key arrives as a string chunk', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + stdin.pressEncoded('\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test.each([ + ['a Buffer', (stdin: FakeStdin) => stdin.press('\u0003')], + ['a string chunk', (stdin: FakeStdin) => stdin.pressEncoded('\u0003')], + ])('rejects with AbortSilentError when Ctrl+C arrives as %s', async (_name, pressCtrlC) => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + pressCtrlC(stdin) + + await expect(promise).rejects.toBeInstanceOf(AbortSilentError) + expect(stdin.listenerCount('data')).toBe(0) + }) + + test('turns raw mode on while it waits and off afterwards when stdin was not raw', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) + + expect(stdin.setRawMode).toHaveBeenLastCalledWith(true) + stdin.press('\r') await promise - expect(mockStdin.unref).toBeCalled() - expect(mockStdin.setRawMode).toHaveBeenLastCalledWith(false) + + expect(stdin.setRawMode).toHaveBeenLastCalledWith(false) }) - test('rejects if sent ctrl+c', async () => { - let registeredListener: any - const mockStdin = { - setRawMode: vi.fn(), - once: (event: string, callback: any) => { - registeredListener = callback - }, - ref: vi.fn(), - unref: vi.fn(), - } as any + test('leaves raw mode on when stdin was already raw', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) - const promise = keypress(mockStdin, {skipTTYCheck: true}) + stdin.press('\r') + await promise - // create a buffer representing pressing ctrl+c - registeredListener(Buffer.from([3])) + expect(stdin.setRawMode).not.toHaveBeenCalled() + expect(stdin.isRaw).toBe(true) + }) - let rejected = false - try { - await promise - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (rejection: any) { - expect(rejection.type).toEqual(FatalErrorType.AbortSilent) - rejected = true - } - expect(rejected).toEqual(true) + test('stops listening to stdin and unrefs it once settled', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) + + expect(stdin.ref).toHaveBeenCalledTimes(1) + expect(stdin.listenerCount('data')).toBe(1) + stdin.press('\r') + await promise + + expect(stdin.listenerCount('data')).toBe(0) + expect(stdin.unref).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index 6f65e8152e5..687e23bcf55 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -740,33 +740,64 @@ export async function renderDangerousConfirmationPrompt( }) } -/** Waits for any key to be pressed except Ctrl+C which will terminate the process. */ +interface KeypressOptions { + /** + * The keys that end the wait, for example `['\r']` for Enter. Other keys are ignored. + * Without this option, any key ends the wait. + */ + keys?: string[] +} -export const keypress = async (stdin = process.stdin, uiDebugOptions: UIDebugOptions = defaultUIDebugOptions) => { +/** + * Waits for a key to be pressed. Ctrl+C rejects with AbortSilentError, which terminates the process. + * + * Raw mode is set back to the state it had before the call. A command that keeps stdin raw for + * its own single-key shortcuts (for example `theme dev`) keeps them. + * + * Chunks are read as text: an active Ink renderer calls `stdin.setEncoding('utf8')`, and then + * stdin emits strings instead of Buffers. + * + * @param stdin - The stream to read from. + * @param uiDebugOptions - Debug options; `skipTTYCheck` allows a non-TTY stdin. + * @param options - The keys that end the wait. + * @returns A promise that resolves when an accepted key is pressed. + */ +export const keypress = async ( + stdin = process.stdin, + uiDebugOptions: UIDebugOptions = defaultUIDebugOptions, + {keys}: KeypressOptions = {}, +) => { throwInNonTTY({message: 'Press any key'}, uiDebugOptions) return runWithTimer('cmd_all_timing_prompts_ms')(() => { - return new Promise((resolve, reject) => { - const handler = (buffer: Buffer) => { - stdin.setRawMode(false) + const {promise, resolve, reject} = Promise.withResolvers() + const stdinWasRaw = stdin.isRaw === true - const bytes = Array.from(buffer) + const stopListening = () => { + stdin.off('data', handler) + if (!stdinWasRaw) stdin.setRawMode(false) + stdin.unref() + } - if (bytes.length && bytes[0] === 3) { - outputDebug('Canceled keypress, User pressed CTRL+C') - reject(new AbortSilentError()) - } - stdin.unref() + const handler = (chunk: Buffer | string) => { + const text = chunk.toString() + if (text.includes('\u0003')) { + stopListening() + outputDebug('Canceled keypress, User pressed CTRL+C') + reject(new AbortSilentError()) + } else if (!keys || [...text].some((character) => keys.includes(character))) { + stopListening() process.nextTick(resolve) } + } - stdin.setRawMode(true) - stdin.once('data', handler) + if (!stdinWasRaw) stdin.setRawMode(true) + stdin.on('data', handler) - // We want to indicate that we're still using stdin, so that the process - // doesn't exit early. - stdin.ref() - }) + // We want to indicate that we're still using stdin, so that the process + // doesn't exit early. + stdin.ref() + return promise }) } diff --git a/packages/theme/src/cli/utilities/errors.test.ts b/packages/theme/src/cli/utilities/errors.test.ts index 9d046017444..6fe28b5a5bd 100644 --- a/packages/theme/src/cli/utilities/errors.test.ts +++ b/packages/theme/src/cli/utilities/errors.test.ts @@ -6,9 +6,11 @@ import { extractFetchErrorInfo, } from './errors.js' import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' -import {AbortError} from '@shopify/cli-kit/node/error' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' +import {recordError} from '@shopify/cli-kit/node/analytics' import {renderError, renderFatalError} from '@shopify/cli-kit/node/ui' +vi.mock('@shopify/cli-kit/node/analytics') vi.mock('@shopify/cli-kit/node/output') vi.mock('@shopify/cli-kit/node/ui') @@ -31,6 +33,14 @@ describe('errors', () => { body: 'test error', }) }) + + test('renders nothing for AbortSilentError', () => { + renderThrownError('Test Headline', new AbortSilentError()) + + expect(renderError).not.toHaveBeenCalled() + expect(renderFatalError).not.toHaveBeenCalled() + expect(recordError).not.toHaveBeenCalled() + }) }) describe('createSyncingCatchError', () => { diff --git a/packages/theme/src/cli/utilities/errors.ts b/packages/theme/src/cli/utilities/errors.ts index 03d98510df5..e52220125a4 100644 --- a/packages/theme/src/cli/utilities/errors.ts +++ b/packages/theme/src/cli/utilities/errors.ts @@ -1,5 +1,5 @@ import {commandEventOutputMode, emitCommandEvent} from '@shopify/cli-kit/node/command-events' -import {AbortError} from '@shopify/cli-kit/node/error' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' import {outputDebug} from '@shopify/cli-kit/node/output' import {recordError} from '@shopify/cli-kit/node/analytics' import {renderError, renderFatalError} from '@shopify/cli-kit/node/ui' @@ -11,6 +11,7 @@ import {createError as createH3Error, type H3Error} from 'h3' * @param error - The error to render. */ export function renderThrownError(headline: string, error: Error | AbortError) { + if (error instanceof AbortSilentError) return recordError(error) if (commandEventOutputMode() === 'json') {