diff --git a/packages/targets/browser-safari/src/index.test.ts b/packages/targets/browser-safari/src/index.test.ts index e1eaf220..a090797f 100644 --- a/packages/targets/browser-safari/src/index.test.ts +++ b/packages/targets/browser-safari/src/index.test.ts @@ -1,15 +1,31 @@ import { fakeBuildContext, fakeShipContext, smokeTest } from '@profullstack/sh1pt-core/testing'; +import { generateKeyPairSync } from 'node:crypto'; import { mkdtemp, readFile, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import adapter from './index.js'; +const childProcessMocks = vi.hoisted(() => ({ + execFileSync: vi.fn(), + execSync: vi.fn(), +})); + +vi.mock('node:child_process', async (importOriginal) => ({ + ...await importOriginal(), + ...childProcessMocks, +})); + smokeTest(adapter, { idPrefix: 'browser', requireKind: true }); const tempDirs: string[] = []; +beforeEach(() => { + vi.clearAllMocks(); +}); + afterEach(async () => { + vi.restoreAllMocks(); await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); }); @@ -81,4 +97,68 @@ describe('Safari extension build planning', () => { bundleId: 'Extension', })).rejects.toThrow('bundleId must look like a reverse-DNS identifier'); }); + + it('passes upload values as literal arguments instead of interpolating a shell command', async () => { + const privateKey = generateKeyPairSync('ec', { namedCurve: 'prime256v1' }) + .privateKey.export({ type: 'pkcs8', format: 'pem' }).toString(); + const fetchMock = vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce(new Response(JSON.stringify({ + data: [{ id: 'app-123', attributes: { name: 'Safe Extension' } }], + }), { status: 200 })) + .mockResolvedValueOnce(new Response('{}', { status: 201 })); + const uploadDir = await mkdtemp(join(tmpdir(), 'sh1pt-safari-upload-')); + tempDirs.push(uploadDir); + const artifact = join(uploadDir, 'release $(touch should-not-run).pkg'); + const appleId = 'publisher+$(touch should-not-run)@example.com'; + + await adapter.ship(fakeShipContext({ + artifact, + dryRun: false, + secret: (key: string) => ({ + APP_STORE_CONNECT_KEY_ID: 'KEY1234567', + APP_STORE_CONNECT_ISSUER_ID: '00000000-0000-0000-0000-000000000000', + APP_STORE_CONNECT_PRIVATE_KEY: privateKey, + })[key], + }) as any, { + bundleId: 'com.acme.Extension', + appleId, + }); + + expect(childProcessMocks.execSync).not.toHaveBeenCalled(); + expect(childProcessMocks.execFileSync).toHaveBeenCalledWith( + 'xcrun', + [ + 'altool', '--upload-app', '-f', artifact, + '-u', appleId, '-p', '@env:APP_STORE_CONNECT_PRIVATE_KEY', + '--type', 'macos', '--output-format', 'json', + ], + expect.objectContaining({ + stdio: 'pipe', + env: expect.objectContaining({ APP_STORE_CONNECT_PRIVATE_KEY: privateKey }), + }), + ); + + fetchMock + .mockResolvedValueOnce(new Response(JSON.stringify({ + data: [{ id: 'app-123', attributes: { name: 'Safe Extension' } }], + }), { status: 200 })) + .mockResolvedValueOnce(new Response('{}', { status: 201 })); + childProcessMocks.execFileSync.mockImplementationOnce(() => { + throw new Error('upload rejected'); + }); + + await expect(adapter.ship(fakeShipContext({ + artifact, + dryRun: false, + secret: (key: string) => ({ + APP_STORE_CONNECT_KEY_ID: 'KEY1234567', + APP_STORE_CONNECT_ISSUER_ID: '00000000-0000-0000-0000-000000000000', + APP_STORE_CONNECT_PRIVATE_KEY: privateKey, + })[key], + }) as any, { + bundleId: 'com.acme.Extension', + appleId, + })).rejects.toThrow('App Store upload failed'); + expect(childProcessMocks.execSync).not.toHaveBeenCalled(); + }); }); diff --git a/packages/targets/browser-safari/src/index.ts b/packages/targets/browser-safari/src/index.ts index 120f0757..d81221a6 100644 --- a/packages/targets/browser-safari/src/index.ts +++ b/packages/targets/browser-safari/src/index.ts @@ -1,5 +1,5 @@ import { defineTarget, manualSetup } from '@profullstack/sh1pt-core'; -import { execFileSync, execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import { createSign } from 'node:crypto'; import { existsSync } from 'node:fs'; import { mkdir, writeFile } from 'node:fs/promises'; @@ -266,24 +266,24 @@ export default defineTarget({ ctx.log('✓ app store version created'); } - // Step 3: Upload the build archive using xcrun altool + // Step 3: Upload the build archive using xcrun altool. Keep every value in + // argv so paths and credentials are never interpreted by a shell. ctx.log('uploading archive with xcrun altool...'); try { - execSync( - `xcrun altool --upload-app -f "${ctx.artifact}" ` + - `-u "${appleId}" -p "@env:APP_STORE_CONNECT_PRIVATE_KEY" ` + - `--type macos --output-format json`, - { stdio: 'pipe', timeout: 600_000 }, - ); - } catch { - ctx.log('altool upload failed, trying notarytool...', 'warn'); - execSync( - `xcrun notarytool submit "${ctx.artifact}" ` + - `--key-id "${keyId}" --issuer "${issuerId}" ` + - `--private-key <(echo "${privateKey}") ` + - `--wait --output-format json`, - { stdio: 'pipe', timeout: 600_000 }, - ); + execFileSync('xcrun', [ + 'altool', '--upload-app', '-f', ctx.artifact, + '-u', appleId, '-p', '@env:APP_STORE_CONNECT_PRIVATE_KEY', + '--type', 'macos', '--output-format', 'json', + ], { + stdio: 'pipe', + timeout: 600_000, + env: { ...process.env, APP_STORE_CONNECT_PRIVATE_KEY: privateKey }, + }); + } catch (error) { + // notarytool notarizes Developer ID artifacts; it does not upload App + // Store builds. Treat an altool failure as a failed upload instead of + // reporting a successful but unrelated notarization. + throw new Error('App Store upload failed: xcrun altool did not complete successfully', { cause: error }); } ctx.log('✓ build uploaded to App Store Connect');