Review fixes for the store signup JWT handoff

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<typeof import('@shopify/cli-kit/node/system')>()),
+  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()
+  })
 })

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,7 @@ export default class StoreStripeAuth extends StoreCommand {
 
   public async run(): Promise<void> {
     const {flags} = await this.parse(StoreStripeAuth)
-    const signup = flags.signup ?? (await readSignupJwtFromStdin())
+    const signup = signupFlagValue(flags.signup) ?? (await readSignupJwtFromStdin())
 
     await authenticateStoreWithApp(
       {
@@ -54,21 +55,39 @@ 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 <jwt>, set SHOPIFY_FLAG_SIGNUP, or pipe the JWT to stdin.'
+
+// A blank --signup is a credential that was never supplied rather than an empty one, so it falls
+// through to stdin instead of starting an authorization without it.
+function signupFlagValue(signup: string | undefined): string | undefined {
+  const trimmed = signup?.trim()
+  return trimmed === '' ? undefined : trimmed
+}
+
 export async function readSignupJwtFromStdin(
   stdin: NodeJS.ReadableStream & AsyncIterable<Buffer | string> = process.stdin,
 ): Promise<string> {
+  // An interactive stdin never ends, so reading it would hang the command instead of reporting the
+  // credential that was never supplied.
+  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 signup JWT piped to stdin is too large.', 'Pipe only the JWT.')
+    }
+    chunks.push(buffer)
   }
 
   const signup = Buffer.concat(chunks).toString('utf8').trim()
-  if (!signup) {
-    throw new AbortError(
-      'Missing signup JWT.',
-      'Pass --signup <jwt>, set SHOPIFY_FLAG_SIGNUP, or pipe the JWT to stdin.',
-    )
-  }
+  if (!signup) throw new AbortError(MISSING_SIGNUP_JWT, MISSING_SIGNUP_JWT_GUIDANCE)
 
   return signup
 }

packages/store/src/cli/services/store/auth/callback.test.ts

@@ -92,6 +92,100 @@ describe('store auth callback server', () => {
     ).resolves.toBe('abc123')
   })
 
+  test('waitForStoreAuthCode answers 404 for a wrong nonce, a replay, and a non-GET handoff request', async () => {
+    const port = await getAvailablePort()
+    const params = callbackParams()
+    const authorizationUrl = 'https://shop.myshopify.com/admin/oauth/authorize?signup=signed.signup.jwt'
+    const handoffUrl = `http://127.0.0.1:${port}/auth/handoff?nonce=nonce-123`
+    const statuses: Record<string, number> = {}
+    const bodies: string[] = []
+
+    const onListening = async () => {
+      const wrongNonce = await globalThis.fetch(`http://127.0.0.1:${port}/auth/handoff?nonce=wrong`, {
+        redirect: 'manual',
+      })
+      statuses.wrongNonce = wrongNonce.status
+      bodies.push(await wrongNonce.text())
+
+      const missingNonce = await globalThis.fetch(`http://127.0.0.1:${port}/auth/handoff`, {redirect: 'manual'})
+      statuses.missingNonce = missingNonce.status
+      bodies.push(await missingNonce.text())
+
+      const notGet = await globalThis.fetch(handoffUrl, {method: 'POST', redirect: 'manual'})
+      statuses.notGet = notGet.status
+      bodies.push(await notGet.text())
+
+      const served = await globalThis.fetch(handoffUrl, {redirect: 'manual'})
+      statuses.served = served.status
+      await served.text()
+
+      const replay = await globalThis.fetch(handoffUrl, {redirect: 'manual'})
+      statuses.replay = replay.status
+      bodies.push(await replay.text())
+
+      const callbackResponse = await globalThis.fetch(`http://127.0.0.1:${port}/auth/callback?${params.toString()}`)
+      await callbackResponse.text()
+    }
+
+    await expect(
+      waitForStoreAuthCode({
+        store: 'shop.myshopify.com',
+        state: 'state-123',
+        port,
+        timeoutMs: 1000,
+        authorizationRedirect: {nonce: 'nonce-123', authorizationUrl},
+        onListening,
+      }),
+    ).resolves.toBe('abc123')
+
+    expect(statuses).toEqual({wrongNonce: 404, missingNonce: 404, notGet: 404, served: 302, replay: 404})
+    expect(bodies.join('')).not.toContain('signed.signup.jwt')
+  })
+
+  test('waitForStoreAuthCode does not spend the handoff on a speculative browser fetch', async () => {
+    const port = await getAvailablePort()
+    const params = callbackParams()
+    const authorizationUrl = 'https://shop.myshopify.com/admin/oauth/authorize?signup=signed.signup.jwt'
+    const handoffUrl = `http://127.0.0.1:${port}/auth/handoff?nonce=nonce-123`
+    let prefetchStatus = 0
+    let prefetchBody = ''
+    let navigationStatus = 0
+    let navigationLocation: string | null = null
+
+    const onListening = async () => {
+      const prefetch = await globalThis.fetch(handoffUrl, {
+        headers: {'Sec-Purpose': 'prefetch;prerender'},
+        redirect: 'manual',
+      })
+      prefetchStatus = prefetch.status
+      prefetchBody = await prefetch.text()
+
+      const navigation = await globalThis.fetch(handoffUrl, {redirect: 'manual'})
+      navigationStatus = navigation.status
+      navigationLocation = navigation.headers.get('Location')
+      await navigation.text()
+
+      const callbackResponse = await globalThis.fetch(`http://127.0.0.1:${port}/auth/callback?${params.toString()}`)
+      await callbackResponse.text()
+    }
+
+    await expect(
+      waitForStoreAuthCode({
+        store: 'shop.myshopify.com',
+        state: 'state-123',
+        port,
+        timeoutMs: 1000,
+        authorizationRedirect: {nonce: 'nonce-123', authorizationUrl},
+        onListening,
+      }),
+    ).resolves.toBe('abc123')
+
+    expect(prefetchStatus).toBe(404)
+    expect(prefetchBody).not.toContain('signed.signup.jwt')
+    expect(navigationStatus).toBe(302)
+    expect(navigationLocation).toBe(authorizationUrl)
+  })
+
   test('waitForStoreAuthCode rejects when callback state does not match', async () => {
     const port = await getAvailablePort()
     const params = callbackParams({state: 'wrong-state'})

packages/store/src/cli/services/store/auth/callback.ts

@@ -5,17 +5,27 @@ 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
   state: string
   port: number
   timeoutMs?: number
   onListening?: () => void | Promise<void>
-  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
       }
 

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(

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)