diff --git a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index dc9eed074bc..bde45320391 100644 --- a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -15,7 +15,7 @@ export type Scalars = { Int: { input: number; output: number; } Float: { input: number; output: number; } AccessRoleAssignee: { input: any; output: any; } - /** The ID for a AccessRole. */ + /** The ID for an AccessRole. */ AccessRoleID: { input: any; output: any; } AccessRoleRecordId: { input: any; output: any; } /** The ID for a ActionAudit. */ diff --git a/packages/organizations/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/organizations/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index 5df36979c6a..58ea0f6d702 100644 --- a/packages/organizations/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/organizations/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -15,7 +15,7 @@ export type Scalars = { Int: { input: number; output: number; } Float: { input: number; output: number; } AccessRoleAssignee: { input: any; output: any; } - /** The ID for a AccessRole. */ + /** The ID for an AccessRole. */ AccessRoleID: { input: any; output: any; } AccessRoleRecordId: { input: any; output: any; } /** The ID for a ActionAudit. */ diff --git a/packages/store/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/store/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index e5ae0715680..e42f3a79c4b 100644 --- a/packages/store/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/store/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -15,7 +15,7 @@ export type Scalars = { Int: { input: number; output: number; } Float: { input: number; output: number; } AccessRoleAssignee: { input: any; output: any; } - /** The ID for a AccessRole. */ + /** The ID for an AccessRole. */ AccessRoleID: { input: any; output: any; } AccessRoleRecordId: { input: any; output: any; } /** The ID for a ActionAudit. */ diff --git a/packages/store/src/cli/commands/store/stripe-auth.test.ts b/packages/store/src/cli/commands/store/stripe-auth.test.ts index aef7e95485a..75512b1eb2c 100644 --- a/packages/store/src/cli/commands/store/stripe-auth.test.ts +++ b/packages/store/src/cli/commands/store/stripe-auth.test.ts @@ -1,6 +1,7 @@ import StoreStripeAuth, {readSignupJwtFromStdin} from './stripe-auth.js' import {authenticateStoreWithApp} from '../../services/store/auth/index.js' import {createStoreAuthPresenter} from '../../services/store/auth/result.js' +import {isStdinPiped} from '@shopify/cli-kit/node/system' import {describe, expect, test, vi} from 'vitest' import {Readable} from 'stream' @@ -9,6 +10,10 @@ vi.mock('../../services/store/attribution.js') vi.mock('../../services/store/auth/result.js', () => ({ createStoreAuthPresenter: vi.fn((format: 'text' | 'json') => ({format})), })) +vi.mock('@shopify/cli-kit/node/system', async (importOriginal) => ({ + ...(await importOriginal()), + isStdinPiped: vi.fn(), +})) describe('store stripe-auth command', () => { test('passes signup JWT through to the auth service', async () => { @@ -71,4 +76,25 @@ describe('store stripe-auth command', () => { test('rejects blank stdin signup JWTs', async () => { await expect(readSignupJwtFromStdin(Readable.from(['\n']))).rejects.toThrow('Missing signup JWT') }) + + test('reports the missing credential instead of waiting when stdin is an interactive terminal', async () => { + vi.mocked(isStdinPiped).mockReturnValue(false) + + await expect(readSignupJwtFromStdin()).rejects.toThrow('Missing signup JWT') + }) + + test('rejects a stdin signup JWT larger than the accepted size', async () => { + const oversized = 'a'.repeat(8 * 1024 + 1) + + await expect(readSignupJwtFromStdin(Readable.from([oversized]))).rejects.toThrow('too large') + }) + + test('does not authenticate when the signup flag is empty and no JWT is piped', async () => { + vi.mocked(isStdinPiped).mockReturnValue(false) + + await expect( + StoreStripeAuth.run(['--store', 'shop.myshopify.com', '--scopes', 'read_products', '--signup', '']), + ).rejects.toThrow() + expect(authenticateStoreWithApp).not.toHaveBeenCalled() + }) }) diff --git a/packages/store/src/cli/commands/store/stripe-auth.ts b/packages/store/src/cli/commands/store/stripe-auth.ts index 48880b1dd20..148189c5a23 100644 --- a/packages/store/src/cli/commands/store/stripe-auth.ts +++ b/packages/store/src/cli/commands/store/stripe-auth.ts @@ -4,6 +4,7 @@ import StoreCommand from '../../utilities/store-command.js' import {storeFlags} from '../../flags.js' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' import {AbortError} from '@shopify/cli-kit/node/error' +import {isStdinPiped} from '@shopify/cli-kit/node/system' import {Flags} from '@oclif/core' export default class StoreStripeAuth extends StoreCommand { @@ -39,7 +40,8 @@ export default class StoreStripeAuth extends StoreCommand { public async run(): Promise { const {flags} = await this.parse(StoreStripeAuth) - const signup = flags.signup ?? (await readSignupJwtFromStdin()) + // A blank --signup counts as not supplied, so the command falls back to reading the JWT from stdin. + const signup = signupFlagValue(flags.signup) ?? (await readSignupJwtFromStdin()) await authenticateStoreWithApp( { @@ -54,21 +56,35 @@ export default class StoreStripeAuth extends StoreCommand { } } +const MAX_SIGNUP_JWT_BYTES = 8 * 1024 +const MISSING_SIGNUP_JWT = 'Missing signup JWT.' +const MISSING_SIGNUP_JWT_GUIDANCE = 'Pass --signup , set SHOPIFY_FLAG_SIGNUP, or pipe the JWT to stdin.' + +function signupFlagValue(signup: string | undefined): string | undefined { + const trimmed = signup?.trim() + return trimmed === '' ? undefined : trimmed +} + export async function readSignupJwtFromStdin( stdin: NodeJS.ReadableStream & AsyncIterable = process.stdin, ): Promise { + if (stdin === process.stdin && !isStdinPiped()) { + throw new AbortError(MISSING_SIGNUP_JWT, MISSING_SIGNUP_JWT_GUIDANCE) + } + const chunks: Buffer[] = [] + let byteLength = 0 for await (const chunk of stdin) { - chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)) + const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk) + byteLength += buffer.length + if (byteLength > MAX_SIGNUP_JWT_BYTES) { + throw new AbortError('The input piped to stdin is too large to be a signup JWT.', 'Pipe only the signup JWT.') + } + chunks.push(buffer) } const signup = Buffer.concat(chunks).toString('utf8').trim() - if (!signup) { - throw new AbortError( - 'Missing signup JWT.', - 'Pass --signup , set SHOPIFY_FLAG_SIGNUP, or pipe the JWT to stdin.', - ) - } + if (!signup) throw new AbortError(MISSING_SIGNUP_JWT, MISSING_SIGNUP_JWT_GUIDANCE) return signup } diff --git a/packages/store/src/cli/services/store/auth/callback.test.ts b/packages/store/src/cli/services/store/auth/callback.test.ts index 133caca5102..2be50623437 100644 --- a/packages/store/src/cli/services/store/auth/callback.test.ts +++ b/packages/store/src/cli/services/store/auth/callback.test.ts @@ -38,6 +38,46 @@ function callbackParams(options?: {code?: string; shop?: string; state?: string; return params } +const handoffNonce = 'nonce-123' +const handoffSignupJwt = 'signed.signup.jwt' +const handoffAuthorizationUrl = `https://shop.myshopify.com/admin/oauth/authorize?signup=${handoffSignupJwt}` + +interface HandoffResponse { + status: number + body: string + headers: Headers +} + +async function fetchHandoff(url: string, init?: Parameters[1]): Promise { + const response = await globalThis.fetch(url, {redirect: 'manual', ...init}) + return {status: response.status, headers: response.headers, body: await response.text()} +} + +// Runs `probe` against a server holding a pending authorization handoff, then completes +// authentication through the callback to prove the probe did not settle or break the auth flow. +async function withPendingHandoff(probe: (handoff: {url: string; port: number}) => Promise): Promise { + const port = await getAvailablePort() + const params = callbackParams() + + const onListening = async () => { + await probe({url: `http://127.0.0.1:${port}/auth/handoff?nonce=${handoffNonce}`, port}) + const callbackResponse = await globalThis.fetch(`http://127.0.0.1:${port}/auth/callback?${params.toString()}`) + expect(callbackResponse.status).toBe(200) + await callbackResponse.text() + } + + await expect( + waitForStoreAuthCode({ + store: 'shop.myshopify.com', + state: 'state-123', + port, + timeoutMs: 1000, + authorizationRedirect: {nonce: handoffNonce, authorizationUrl: handoffAuthorizationUrl}, + onListening, + }), + ).resolves.toBe('abc123') +} + describe('store auth callback server', () => { test('waitForStoreAuthCode resolves after a valid callback', async () => { const port = await getAvailablePort() @@ -60,36 +100,60 @@ describe('store auth callback server', () => { }) test('waitForStoreAuthCode redirects a valid authorization handoff without settling auth', async () => { - const port = await getAvailablePort() - const params = callbackParams() - const authorizationUrl = 'https://shop.myshopify.com/admin/oauth/authorize?signup=signed.signup.jwt' - const onListening = async () => { - const handoffResponse = await globalThis.fetch(`http://127.0.0.1:${port}/auth/handoff?nonce=nonce-123`, { - redirect: 'manual', - }) - expect(handoffResponse.status).toBe(302) - expect(handoffResponse.headers.get('Location')).toBe(authorizationUrl) - expect(handoffResponse.headers.get('Cache-Control')).toBe('no-store') - expect(handoffResponse.headers.get('Referrer-Policy')).toBe('no-referrer') - - const callbackResponse = await globalThis.fetch(`http://127.0.0.1:${port}/auth/callback?${params.toString()}`) - expect(callbackResponse.status).toBe(200) - await callbackResponse.text() - } + await withPendingHandoff(async ({url}) => { + const handoff = await fetchHandoff(url) + expect(handoff.status).toBe(302) + expect(handoff.headers.get('Location')).toBe(handoffAuthorizationUrl) + expect(handoff.headers.get('Cache-Control')).toBe('no-store') + expect(handoff.headers.get('Referrer-Policy')).toBe('no-referrer') + }) + }) - await expect( - waitForStoreAuthCode({ - store: 'shop.myshopify.com', - state: 'state-123', - port, - timeoutMs: 1000, - authorizationRedirect: { - nonce: 'nonce-123', - authorizationUrl, - }, - onListening, - }), - ).resolves.toBe('abc123') + test('waitForStoreAuthCode answers 404 to a handoff request with a wrong nonce', async () => { + await withPendingHandoff(async ({port}) => { + const handoff = await fetchHandoff(`http://127.0.0.1:${port}/auth/handoff?nonce=wrong`) + expect(handoff.status).toBe(404) + expect(handoff.body).not.toContain(handoffSignupJwt) + }) + }) + + test('waitForStoreAuthCode answers 404 to a handoff request with no nonce', async () => { + await withPendingHandoff(async ({port}) => { + const handoff = await fetchHandoff(`http://127.0.0.1:${port}/auth/handoff`) + expect(handoff.status).toBe(404) + expect(handoff.body).not.toContain(handoffSignupJwt) + }) + }) + + test('waitForStoreAuthCode answers 404 to a non-GET handoff request', async () => { + await withPendingHandoff(async ({url}) => { + const handoff = await fetchHandoff(url, {method: 'POST'}) + expect(handoff.status).toBe(404) + expect(handoff.body).not.toContain(handoffSignupJwt) + }) + }) + + test('waitForStoreAuthCode answers 404 to a replayed handoff request', async () => { + await withPendingHandoff(async ({url}) => { + const served = await fetchHandoff(url) + expect(served.status).toBe(302) + + const replay = await fetchHandoff(url) + expect(replay.status).toBe(404) + expect(replay.body).not.toContain(handoffSignupJwt) + }) + }) + + test('waitForStoreAuthCode does not spend the handoff on a speculative browser fetch', async () => { + await withPendingHandoff(async ({url}) => { + const prefetch = await fetchHandoff(url, {headers: {'Sec-Purpose': 'prefetch;prerender'}}) + expect(prefetch.status).toBe(404) + expect(prefetch.body).not.toContain(handoffSignupJwt) + + const navigation = await fetchHandoff(url) + expect(navigation.status).toBe(302) + expect(navigation.headers.get('Location')).toBe(handoffAuthorizationUrl) + }) }) test('waitForStoreAuthCode rejects when callback state does not match', async () => { diff --git a/packages/store/src/cli/services/store/auth/callback.ts b/packages/store/src/cli/services/store/auth/callback.ts index eacf4b95de0..3331a08149b 100644 --- a/packages/store/src/cli/services/store/auth/callback.ts +++ b/packages/store/src/cli/services/store/auth/callback.ts @@ -5,6 +5,12 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' import {timingSafeEqual} from 'crypto' import {createServer} from 'http' +import type {IncomingHttpHeaders} from 'http' + +export interface AuthorizationRedirect { + nonce: string + authorizationUrl: string +} export interface WaitForAuthCodeOptions { store: string @@ -12,10 +18,14 @@ export interface WaitForAuthCodeOptions { port: number timeoutMs?: number onListening?: () => void | Promise - authorizationRedirect?: { - nonce: string - authorizationUrl: string - } + authorizationRedirect?: AuthorizationRedirect +} + +// Browsers announce a speculative fetch so servers can decline side effects. Serving one would spend +// the single-use handoff before the navigation it is speculating about ever arrives. +function isSpeculativeRequest(headers: IncomingHttpHeaders): boolean { + const purpose = [headers['sec-purpose'], headers.purpose, headers['x-moz']].flat().join(' ') + return purpose.includes('prefetch') || purpose.includes('prerender') } function renderAuthCallbackPage(title: string, message: string): string { @@ -119,21 +129,26 @@ export async function waitForStoreAuthCode({ const server = createServer((req, res) => { const requestUrl = new URL(req.url ?? '/', `http://127.0.0.1:${port}`) - if (requestUrl.pathname === STORE_AUTH_HANDOFF_PATH && authorizationRedirect) { - const returnedNonce = requestUrl.searchParams.get('nonce') - if (!returnedNonce || !constantTimeEqual(returnedNonce, authorizationRedirect.nonce)) { - res.statusCode = 403 - res.setHeader('Cache-Control', 'no-store') - res.setHeader('Connection', 'close') - res.end('Forbidden') - return - } + const notFound = () => { + res.statusCode = 404 + res.setHeader('Cache-Control', 'no-store') + res.setHeader('Connection', 'close') + res.end('Not found') + } - if (authorizationRedirectUsed) { - res.statusCode = 410 - res.setHeader('Cache-Control', 'no-store') - res.setHeader('Connection', 'close') - res.end('Authorization handoff already used') + if (requestUrl.pathname === STORE_AUTH_HANDOFF_PATH) { + const returnedNonce = requestUrl.searchParams.get('nonce') + // Every rejection answers 404 so a local prober cannot tell a wrong nonce from a spent + // handoff, or either from a port with no store auth in flight. + const servable = + authorizationRedirect !== undefined && + !authorizationRedirectUsed && + req.method === 'GET' && + returnedNonce !== null && + constantTimeEqual(returnedNonce, authorizationRedirect.nonce) + + if (!servable || isSpeculativeRequest(req.headers)) { + notFound() return } @@ -148,8 +163,7 @@ export async function waitForStoreAuthCode({ } if (requestUrl.pathname !== STORE_AUTH_CALLBACK_PATH) { - res.statusCode = 404 - res.end('Not found') + notFound() return } diff --git a/packages/store/src/cli/services/store/auth/index.test.ts b/packages/store/src/cli/services/store/auth/index.test.ts index 63dad5722a1..24f15316b5d 100644 --- a/packages/store/src/cli/services/store/auth/index.test.ts +++ b/packages/store/src/cli/services/store/auth/index.test.ts @@ -280,7 +280,7 @@ describe('store auth service', () => { const openURL = vi.fn().mockResolvedValue(false) const presenter = { openingBrowser: vi.fn(), - manualAuthUrl: vi.fn(), + manualAuthUrl: vi.fn().mockReturnValue(true), success: vi.fn(), } const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { @@ -309,7 +309,6 @@ describe('store auth service', () => { expect(presenter.openingBrowser).toHaveBeenCalledOnce() expect(presenter.manualAuthUrl).toHaveBeenCalledWith( expect.stringContaining('https://shop.myshopify.com/admin/oauth/authorize?'), - {sensitive: false}, ) expect(presenter.success).toHaveBeenCalledWith(result) }) @@ -318,7 +317,7 @@ describe('store auth service', () => { const openURL = vi.fn().mockResolvedValue(false) const presenter = { openingBrowser: vi.fn(), - manualAuthUrl: vi.fn(), + manualAuthUrl: vi.fn().mockReturnValue(true), success: vi.fn(), } const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { @@ -347,11 +346,43 @@ describe('store auth service', () => { expect(presenter.manualAuthUrl).toHaveBeenCalledWith( expect.stringContaining('http://127.0.0.1:13387/auth/handoff?nonce='), - {sensitive: false}, ) expect(presenter.manualAuthUrl.mock.calls[0]![0]).not.toContain('signed.signup.jwt') }) + test('authenticateStoreWithApp fails immediately when the presenter withholds the authorization URL', async () => { + const openURL = vi.fn().mockResolvedValue(false) + const presenter = { + openingBrowser: vi.fn(), + manualAuthUrl: vi.fn().mockReturnValue(false), + success: vi.fn(), + } + const exchangeStoreAuthCodeForToken = vi.fn() + const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => { + await options.onListening?.() + return 'abc123' + }) + + await expect( + authenticateStoreWithApp( + { + store: 'shop.myshopify.com', + scopes: 'read_products', + signup: 'signed.signup.jwt', + }, + { + openURL, + waitForStoreAuthCode: waitForStoreAuthCodeMock, + exchangeStoreAuthCodeForToken, + presenter, + }, + ), + ).rejects.toThrow("Authentication can't continue without a browser.") + + expect(exchangeStoreAuthCodeForToken).not.toHaveBeenCalled() + expect(presenter.success).not.toHaveBeenCalled() + }) + test('authenticateStoreWithApp records fqdn metadata before resolving existing scopes', async () => { await expect( authenticateStoreWithApp( diff --git a/packages/store/src/cli/services/store/auth/index.ts b/packages/store/src/cli/services/store/auth/index.ts index dbf6443f345..ea7e5962293 100644 --- a/packages/store/src/cli/services/store/auth/index.ts +++ b/packages/store/src/cli/services/store/auth/index.ts @@ -76,7 +76,12 @@ export async function authenticateStoreWithApp( ...bootstrap.waitForAuthCodeOptions, onListening: async () => { const opened = await resolvedDependencies.openURL(authorizationUrl) - if (!opened) resolvedDependencies.presenter.manualAuthUrl(authorizationUrl, {sensitive: false}) + if (opened) return + + // The callback server can only be reached by a browser that was given a URL, so waiting after a + // withheld one would idle until the timeout instead of reporting that authentication cannot proceed. + const surfaced = resolvedDependencies.presenter.manualAuthUrl(authorizationUrl) + if (!surfaced) throw new AbortError("Authentication can't continue without a browser.") }, }) const tokenResponse = await bootstrap.exchangeCodeForToken(code) diff --git a/packages/store/src/cli/services/store/auth/pkce.test.ts b/packages/store/src/cli/services/store/auth/pkce.test.ts index bb126f25d59..405e1c763ff 100644 --- a/packages/store/src/cli/services/store/auth/pkce.test.ts +++ b/packages/store/src/cli/services/store/auth/pkce.test.ts @@ -20,19 +20,28 @@ describe('store auth PKCE helpers', () => { expect(computeCodeChallenge(verifier)).toBe(expected) }) - test('buildStoreAuthUrl includes signup JWT when provided', () => { - const url = new URL( - buildStoreAuthUrl({ - store: 'shop.myshopify.com', - scopes: ['read_products'], - state: 'state-123', - redirectUri: 'http://127.0.0.1:13387/auth/callback', - codeChallenge: 'test-challenge-value', - signup: 'signed.signup.jwt', - }), - ) + test('buildStoreAuthUrl builds a store authorization URL that carries no signup credential', () => { + const url = buildStoreAuthUrl({ + store: 'shop.myshopify.com', + scopes: ['read_products'], + state: 'state-123', + redirectUri: 'http://127.0.0.1:13387/auth/callback', + codeChallenge: 'test-challenge-value', + }) + + expect(new URL(url).searchParams.has('signup')).toBe(false) + expect(url).not.toContain('signup') + }) + + test('createPkceBootstrap keeps the signup credential off the authorization context', () => { + const bootstrap = createPkceBootstrap({ + store: 'shop.myshopify.com', + scopes: ['read_products'], + signup: 'signed.signup.jwt', + exchangeCodeForToken: async () => ({access_token: 'token', scope: 'read_products'}), + }) - expect(url.searchParams.get('signup')).toBe('signed.signup.jwt') + expect(JSON.stringify(bootstrap.authorization)).not.toContain('signed.signup.jwt') }) test('createPkceBootstrap uses a loopback handoff URL when signup is provided', () => { diff --git a/packages/store/src/cli/services/store/auth/pkce.ts b/packages/store/src/cli/services/store/auth/pkce.ts index be97728fdf7..f1fd55372a4 100644 --- a/packages/store/src/cli/services/store/auth/pkce.ts +++ b/packages/store/src/cli/services/store/auth/pkce.ts @@ -3,7 +3,7 @@ import {randomUUID} from '@shopify/cli-kit/node/crypto' import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output' import {createHash, randomBytes} from 'crypto' import type {StoreTokenResponse} from './token-client.js' -import type {WaitForAuthCodeOptions} from './callback.js' +import type {AuthorizationRedirect, WaitForAuthCodeOptions} from './callback.js' interface StoreAuthorizationContext { store: string @@ -13,7 +13,6 @@ interface StoreAuthorizationContext { redirectUri: string authorizationUrl: string codeVerifier: string - signup?: string } interface StoreAuthBootstrap { @@ -36,7 +35,6 @@ export function buildStoreAuthUrl(options: { state: string redirectUri: string codeChallenge: string - signup?: string }): string { const params = new URLSearchParams() params.set('client_id', STORE_AUTH_APP_CLIENT_ID) @@ -46,11 +44,17 @@ export function buildStoreAuthUrl(options: { params.set('response_type', 'code') params.set('code_challenge', options.codeChallenge) params.set('code_challenge_method', 'S256') - if (options.signup) params.set('signup', options.signup) return `https://${options.store}/admin/oauth/authorize?${params.toString()}` } +function buildAuthorizationRedirect(storeAuthorizationUrl: string, signup: string): AuthorizationRedirect { + const authorizationUrl = new URL(storeAuthorizationUrl) + authorizationUrl.searchParams.set('signup', signup) + + return {nonce: randomBytes(32).toString('base64url'), authorizationUrl: authorizationUrl.toString()} +} + export function createPkceBootstrap(options: { store: string scopes: string[] @@ -68,9 +72,11 @@ export function createPkceBootstrap(options: { const redirectUri = storeAuthRedirectUri(port) const codeVerifier = generateCodeVerifier() const codeChallenge = computeCodeChallenge(codeVerifier) - const sensitiveAuthorizationUrl = buildStoreAuthUrl({store, scopes, state, redirectUri, codeChallenge, signup}) - const handoffNonce = signup ? randomBytes(32).toString('base64url') : undefined - const authorizationUrl = handoffNonce ? storeAuthHandoffUri(port, handoffNonce) : sensitiveAuthorizationUrl + const storeAuthorizationUrl = buildStoreAuthUrl({store, scopes, state, redirectUri, codeChallenge}) + const authorizationRedirect = signup ? buildAuthorizationRedirect(storeAuthorizationUrl, signup) : undefined + const authorizationUrl = authorizationRedirect + ? storeAuthHandoffUri(port, authorizationRedirect.nonce) + : storeAuthorizationUrl outputDebug( outputContent`Starting PKCE auth for ${outputToken.raw(store)} with scopes ${outputToken.raw(scopes.join(','))} (redirect_uri=${outputToken.raw(redirectUri)})`, @@ -85,18 +91,12 @@ export function createPkceBootstrap(options: { redirectUri, authorizationUrl, codeVerifier, - signup, }, waitForAuthCodeOptions: { store, state, port, - authorizationRedirect: handoffNonce - ? { - nonce: handoffNonce, - authorizationUrl: sensitiveAuthorizationUrl, - } - : undefined, + authorizationRedirect, }, exchangeCodeForToken: (code: string) => exchangeCodeForToken({store, code, codeVerifier, redirectUri}), } diff --git a/packages/store/src/cli/services/store/auth/result.test.ts b/packages/store/src/cli/services/store/auth/result.test.ts index f4230b34485..8ccad9d1e93 100644 --- a/packages/store/src/cli/services/store/auth/result.test.ts +++ b/packages/store/src/cli/services/store/auth/result.test.ts @@ -113,9 +113,12 @@ describe('store auth presenter', () => { const output = mockAndCaptureOutput() const presenter = createStoreAuthPresenter('text') - presenter.manualAuthUrl('https://shop.myshopify.com/admin/oauth/authorize?client_id=test&secret=sensitive', { - sensitive: true, - }) + const surfaced = presenter.manualAuthUrl( + 'https://shop.myshopify.com/admin/oauth/authorize?client_id=test&secret=sensitive', + {sensitive: true}, + ) + + expect(surfaced).toBe(false) expect(output.info()).toContain( 'Browser did not open automatically. The manual authorization URL contains sensitive credentials and was not printed.', @@ -126,4 +129,47 @@ describe('store auth presenter', () => { expect(output.info()).not.toContain('secret=sensitive') expect(output.info()).not.toContain('https://shop.myshopify.com/admin/oauth/authorize') }) + + test('withholds a manual auth URL carrying a signup credential even when the caller does not mark it sensitive', () => { + const output = mockAndCaptureOutput() + const presenter = createStoreAuthPresenter('text') + + const surfaced = presenter.manualAuthUrl( + 'https://shop.myshopify.com/admin/oauth/authorize?client_id=test&signup=signed.signup.jwt', + ) + + expect(surfaced).toBe(false) + + expect(output.info()).toContain( + 'Browser did not open automatically. The manual authorization URL contains sensitive credentials and was not printed.', + ) + expect(output.info()).not.toContain('signed.signup.jwt') + expect(output.info()).not.toContain('signup=') + }) + + test('withholds a manual auth URL it cannot parse', () => { + const output = mockAndCaptureOutput() + const presenter = createStoreAuthPresenter('text') + + const surfaced = presenter.manualAuthUrl('not-a-url?signup=signed.signup.jwt') + + expect(surfaced).toBe(false) + + expect(output.info()).toContain( + 'Browser did not open automatically. The manual authorization URL contains sensitive credentials and was not printed.', + ) + expect(output.info()).not.toContain('signed.signup.jwt') + }) + + test('prints a loopback handoff URL that carries no credential', () => { + const output = mockAndCaptureOutput() + const presenter = createStoreAuthPresenter('text') + + const surfaced = presenter.manualAuthUrl('http://127.0.0.1:13387/auth/handoff?nonce=abc123') + + expect(surfaced).toBe(true) + + expect(output.info()).toContain('Browser did not open automatically. Open this URL manually:') + expect(output.info()).toContain('http://127.0.0.1:13387/auth/handoff?nonce=abc123') + }) }) diff --git a/packages/store/src/cli/services/store/auth/result.ts b/packages/store/src/cli/services/store/auth/result.ts index db83df4a277..59d20acb1d8 100644 --- a/packages/store/src/cli/services/store/auth/result.ts +++ b/packages/store/src/cli/services/store/auth/result.ts @@ -25,7 +25,7 @@ interface ManualAuthUrlOptions { export interface StoreAuthPresenter { openingBrowser: () => void - manualAuthUrl: (authorizationUrl: string, options?: ManualAuthUrlOptions) => void + manualAuthUrl: (authorizationUrl: string, options?: ManualAuthUrlOptions) => boolean success: (result: StoreAuthResult) => void } @@ -51,19 +51,28 @@ function displayStoreAuthOpeningBrowser(): void { outputInfo('') } -function displayStoreAuthManualAuthUrl(authorizationUrl: string, options: ManualAuthUrlOptions = {}): void { - if (options.sensitive) { +// Callers mark a URL sensitive when they know why it is; this catches the signup credential even when +// they forget, and fails closed on anything it cannot parse well enough to clear. +function carriesSignupCredential(authorizationUrl: string): boolean { + if (!URL.canParse(authorizationUrl)) return true + return new URL(authorizationUrl).searchParams.has('signup') +} + +function displayStoreAuthManualAuthUrl(authorizationUrl: string, options: ManualAuthUrlOptions = {}): boolean { + if (options.sensitive || carriesSignupCredential(authorizationUrl)) { outputInfo( 'Browser did not open automatically. The manual authorization URL contains sensitive credentials and was not printed.', ) outputInfo('Run this command again in an environment where Shopify CLI can open a browser automatically.') outputInfo('') - return + return false } outputInfo('Browser did not open automatically. Open this URL manually:') outputInfo(outputContent`${outputToken.link(authorizationUrl)}`) outputInfo('') + + return true } function displayStoreAuthResult(result: StoreAuthResult, format: StoreAuthOutputFormat = 'text'): void {