fix(safari): avoid shell interpolation during upload

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<typeof import('node:child_process')>(),
+  ...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();
+  });
 });

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<Config>({
       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');