diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b9575f96dbf4..3751f5f2b289 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -1213,6 +1213,7 @@ jobs: - name: Test Windows-specific boundaries run: >- pnpm exec vitest run --config config/vitest.config.ts + src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts config/scripts/rebuild-native-deps.test.mjs config/scripts/rebuild-native-deps-windows-process-tree.test.mjs config/scripts/rebuild-native-deps-node-pty.test.mjs diff --git a/.github/workflows/zcode-credential-contract.yml b/.github/workflows/zcode-credential-contract.yml new file mode 100644 index 000000000000..3e991b22a4e2 --- /dev/null +++ b/.github/workflows/zcode-credential-contract.yml @@ -0,0 +1,37 @@ +name: Coding Plan credential contract +on: + pull_request: + paths: + - 'src/main/zcode/**' + - 'src/shared/secure-file*' + - 'src/shared/secure-path-windows-acl*' + - 'src/shared/windows-security-descriptor*' + - '.github/workflows/zcode-credential-contract.yml' + workflow_dispatch: +permissions: + contents: read +concurrency: + group: coding-plan-contract-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true +jobs: + credential-files: + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04, macos-15, windows-2022] + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + env: + ORCA_BACKGROUND_LAUNCH: '1' + steps: + - uses: actions/checkout@v6 + with: + persist-credentials: false + - uses: ./.github/actions/install-node-dependencies + with: + native-runtime: node + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + with: + bun-version-file: config/.bun-version + - name: Verify rollback and native Windows permissions + run: pnpm test src/main/zcode/zcode-plan-api-key-store.test.ts src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts src/shared/secure-file.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index ddc84b4efb62..1d1d784db863 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -360,6 +360,7 @@ const LINUX_PACKAGE_TESTS = [ ] const WINDOWS_PACKAGE_TESTS = [ + 'src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts', ...LINUX_PACKAGE_TESTS, 'config/scripts/rebuild-native-deps.test.mjs', 'config/scripts/rebuild-native-deps-windows-process-tree.test.mjs', diff --git a/src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts b/src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts new file mode 100644 index 000000000000..7e069327e896 --- /dev/null +++ b/src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts @@ -0,0 +1,141 @@ +import * as fs from 'node:fs' +import * as os from 'node:os' +import { join } from 'node:path' +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest' +import type * as WindowsAcl from '../../shared/secure-path-windows-acl' +import type * as Store from './zcode-plan-api-key-store' + +type NativeCase = { + home: string + fault: 'none' | 'before-restore' | 'after-restore' | 'competing' + reject: boolean + writes: number + renames: number + stages: number + nativeOutcomes: boolean[] + directories: boolean[] +} +const state = vi.hoisted((): NativeCase => ({ + home: '', + fault: 'none', + reject: false, + writes: 0, + renames: 0, + stages: 0, + nativeOutcomes: [], + directories: [] +})) +vi.mock('electron', () => ({ safeStorage: { isEncryptionAvailable: () => false } })) +vi.mock('node:os', async (original) => ({ + ...(await original()), + homedir: () => state.home +})) +vi.mock('node:fs', async (original) => { + const real = await original() + return { + ...real, + writeFileSync: (...args: Parameters) => { + if (++state.writes === 2 && state.fault === 'before-restore') { + throw new Error('Synthetic restore staging failure') + } + return real.writeFileSync(...args) + }, + renameSync: (...args: Parameters) => { + real.renameSync(...args) + if (++state.renames === 2 && state.fault === 'after-restore') { + throw new Error('Synthetic failure after restore publication') + } + } + } +}) +vi.mock('../../shared/secure-path-windows-acl', async (original) => { + const real = await original() + const files = await vi.importActual('node:fs') + return { + ...real, + restrictWindowsPathSync: (...args: Parameters) => { + const outcome = real.restrictWindowsPathSync(...args) + state.nativeOutcomes.push(outcome) + if (state.fault === 'competing' && args[0].endsWith('.tmp') && ++state.stages === 2) { + const competing = join(state.home, '.orca', 'competing-key') + files.writeFileSync(competing, envelope('synthetic-competing')) + expect(real.restrictWindowsPathSync(competing, false)).toBe(true) + files.renameSync(competing, join(state.home, '.orca', 'zcode-plan-api-key.enc')) + } + return state.reject ? false : outcome + }, + bestEffortRestrictWindowsPath: ( + path: string, + directory: boolean, + settled?: (restricted: boolean) => void + ) => + real.bestEffortRestrictWindowsPath(path, directory, (restricted) => { + state.directories.push(restricted) + settled?.(restricted) + }) + } +}) + +const envelope = (key: string) => + `orca-zcode-plan-api-key:v1:plaintext:${Buffer.from(key).toString('base64')}` +let store: typeof Store +let keyPath: string + +describe.skipIf(process.platform !== 'win32')('native Windows Coding Plan rollback', () => { + beforeEach(async () => { + state.home = fs.mkdtempSync(join(os.tmpdir(), 'orca-native-plan-')) + state.fault = 'none' + state.reject = false + state.writes = 0 + state.renames = 0 + state.stages = 0 + state.nativeOutcomes = [] + state.directories = [] + keyPath = join(state.home, '.orca', 'zcode-plan-api-key.enc') + vi.spyOn(console, 'warn').mockImplementation(() => undefined) + vi.resetModules() + store = await import('./zcode-plan-api-key-store') + store.saveZcodePlanApiKey('synthetic-previous') + expect(fs.readFileSync(keyPath, 'utf8')).toBe(envelope('synthetic-previous')) + expect(state.nativeOutcomes.length).toBeGreaterThan(0) + expect(state.nativeOutcomes.every(Boolean)).toBe(true) + await vi.waitFor( + () => { + expect(state.directories.length).toBeGreaterThan(0) + expect(state.directories.every(Boolean)).toBe(true) + }, + { timeout: 15_000 } + ) + state.writes = 0 + state.renames = 0 + state.stages = 0 + state.reject = true + }) + + afterEach(() => { + vi.restoreAllMocks() + fs.rmSync(state.home, { recursive: true, force: true }) + }) + + it.each(['before-restore', 'after-restore', 'competing'] as const)( + 'preserves the file contract after %s with real file and directory ACLs', + (fault) => { + state.fault = fault + expect(() => store.saveZcodePlanApiKey('synthetic-rejected')).toThrow( + 'could not be stored securely' + ) + if (fault === 'before-restore') { + expect(fs.existsSync(keyPath)).toBe(false) + } else { + expect(fs.readFileSync(keyPath, 'utf8')).toBe( + envelope(fault === 'competing' ? 'synthetic-competing' : 'synthetic-previous') + ) + } + expect(fs.readdirSync(join(state.home, '.orca')).some((name) => name.endsWith('.tmp'))).toBe( + false + ) + expect(state.nativeOutcomes.every(Boolean)).toBe(true) + }, + 30_000 + ) +}) diff --git a/src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts b/src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts new file mode 100644 index 000000000000..5784b8a0a6d6 --- /dev/null +++ b/src/main/zcode/zcode-plan-api-key-store-plaintext-rollback.test.ts @@ -0,0 +1,342 @@ +import type * as fs from 'node:fs' +import * as os from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as SecureFile from '../../shared/secure-file' +import type * as ZcodePlanApiKeyStore from './zcode-plan-api-key-store' + +const mocks = vi.hoisted(() => ({ + home: '', + write: vi.fn(), + rename: vi.fn(), + restrict: vi.fn((_path: string, _directory: boolean) => false) +})) + +vi.mock('electron', () => ({ safeStorage: { isEncryptionAvailable: () => false } })) +vi.mock('node:os', async (original) => ({ + ...(await original()), + homedir: () => mocks.home +})) +vi.mock('node:fs', async (original) => ({ + ...(await original()), + writeFileSync: mocks.write, + renameSync: mocks.rename +})) +vi.mock('../../shared/secure-path-windows-acl', () => ({ + restrictWindowsPathSync: mocks.restrict, + bestEffortRestrictWindowsPath: ( + _path: string, + _directory: boolean, + settled: (restricted: boolean) => void + ) => settled(false), + resetSecureFileWindowsUserSidForTests: vi.fn() +})) + +const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')! +const realFs = await vi.importActual('node:fs') +const envelope = (value: string): string => + `orca-zcode-plan-api-key:v1:plaintext:${Buffer.from(value).toString('base64')}` +let store: typeof ZcodePlanApiKeyStore +let writer: typeof SecureFile +let keyPath: string + +beforeEach(async () => { + mocks.home = realFs.mkdtempSync(join(os.tmpdir(), 'orca-glm-plaintext-rollback-')) + keyPath = join(mocks.home, '.orca', 'zcode-plan-api-key.enc') + realFs.mkdirSync(join(mocks.home, '.orca')) + mocks.write.mockReset().mockImplementation(realFs.writeFileSync) + mocks.rename.mockReset().mockImplementation(realFs.renameSync) + mocks.restrict.mockReset().mockReturnValue(false) + vi.spyOn(console, 'warn').mockImplementation(() => undefined) + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + vi.resetModules() + store = await import('./zcode-plan-api-key-store') + writer = await import('../../shared/secure-file') +}) + +afterEach(() => { + Object.defineProperty(process, 'platform', platformDescriptor) + vi.restoreAllMocks() + realFs.rmSync(mocks.home, { recursive: true, force: true }) +}) + +function rejectReplacement(): void { + expect(() => store.saveZcodePlanApiKey('new-key')).toThrow( + 'could not be stored securely on this device' + ) +} + +describe('plaintext rollback through the secure-file writer', () => { + it('removes only the rejected replacement when rollback fails before publication', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.write.mockImplementationOnce(realFs.writeFileSync).mockImplementationOnce(() => { + throw new Error('Synthetic rollback staging failure') + }) + + rejectReplacement() + + expect(mocks.write).toHaveBeenCalledTimes(2) + expect(realFs.existsSync(keyPath)).toBe(false) + expect(store.readZcodePlanApiKey()).toBeNull() + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual([]) + }) + + it.each(['restriction-false', 'restriction-throws'])( + 'keeps the previous envelope if rollback publishes before %s', + (failure) => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.restrict.mockImplementation((path) => { + if ( + failure === 'restriction-throws' && + path === keyPath && + mocks.write.mock.calls.length === 2 + ) { + throw new Error('Synthetic restored-path restriction failure') + } + return false + }) + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('previous-key')) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + } + ) + + it.each([ + { phase: 'before-rollback', previous: true }, + { phase: 'before-rollback', previous: false }, + { phase: 'during-rollback', previous: true } + ])( + 'preserves a different envelope written $phase with previous key $previous', + ({ phase, previous }) => { + if (previous) { + realFs.writeFileSync(keyPath, envelope('previous-key')) + } + if (phase === 'before-rollback') { + mocks.restrict.mockImplementation((path) => { + if (path === keyPath && mocks.write.mock.calls.length === 1) { + realFs.writeFileSync(keyPath, envelope('other-writer-key')) + } + return false + }) + } else { + mocks.write.mockImplementationOnce(realFs.writeFileSync).mockImplementationOnce(() => { + realFs.writeFileSync(keyPath, envelope('other-writer-key')) + throw new Error('Synthetic competing rollback writer') + }) + } + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('other-writer-key')) + expect(store.readZcodePlanApiKey()).toBe('other-writer-key') + } + ) + + it('retains an identical previous plaintext envelope when retry rollback cannot publish', () => { + realFs.writeFileSync(keyPath, envelope('new-key')) + mocks.write.mockImplementationOnce(realFs.writeFileSync).mockImplementationOnce(() => { + throw new Error('Synthetic same-key rollback staging failure') + }) + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('new-key')) + expect(store.readZcodePlanApiKey()).toBe('new-key') + }) + + it('removes a new unrestricted envelope without caching it when no previous key exists', () => { + rejectReplacement() + + expect(realFs.existsSync(keyPath)).toBe(false) + expect(store.readZcodePlanApiKey()).toBeNull() + }) + + it('keeps the cached previous key after a rejected replacement', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + mocks.write.mockImplementationOnce(realFs.writeFileSync).mockImplementationOnce(() => { + throw new Error('Synthetic rollback staging failure') + }) + + rejectReplacement() + + expect(store.readZcodePlanApiKey()).toBe('previous-key') + }) +}) + +describe('conditional rollback publication through the real writer', () => { + it.each([false, true])( + 'preserves a competing envelope during rollback staging with restriction %s', + (restricted) => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.restrict.mockImplementation((path) => { + if (path.endsWith('.tmp') && mocks.write.mock.calls.length === 2) { + realFs.writeFileSync(keyPath, envelope('other-writer-key')) + return restricted + } + return false + }) + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('other-writer-key')) + expect(store.readZcodePlanApiKey()).toBe('other-writer-key') + expect(mocks.rename).toHaveBeenCalledTimes(1) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + } + ) + + it('keeps the cached previous key when a competing staged rollback cancels publication', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + mocks.restrict.mockImplementation((path) => { + if (path.endsWith('.tmp') && mocks.write.mock.calls.length === 2) { + realFs.writeFileSync(keyPath, envelope('other-writer-key')) + } + return false + }) + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('other-writer-key')) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + expect(mocks.rename).toHaveBeenCalledTimes(1) + }) + + it('preserves an unowned envelope if rollback staging restriction throws', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.restrict.mockImplementation((path) => { + if (path.endsWith('.tmp') && mocks.write.mock.calls.length === 2) { + realFs.writeFileSync(keyPath, envelope('other-writer-key')) + throw new Error('Synthetic staged restriction failure') + } + return false + }) + + rejectReplacement() + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('other-writer-key')) + expect(store.readZcodePlanApiKey()).toBe('other-writer-key') + expect(mocks.rename).toHaveBeenCalledTimes(1) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) + + it('removes only the owned rejected plaintext when rollback rename fails', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.rename.mockImplementationOnce(realFs.renameSync).mockImplementationOnce(() => { + throw new Error('Synthetic rollback rename failure') + }) + + rejectReplacement() + + expect(realFs.existsSync(keyPath)).toBe(false) + expect(store.readZcodePlanApiKey()).toBeNull() + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual([]) + }) + + it.each([false, true])( + 'cancels guarded durable publication without changing a previously present target: %s', + (previous) => { + if (previous) { + realFs.writeFileSync(keyPath, envelope('previous-key')) + } + const shouldPublish = vi.fn(() => false) + + expect( + writer.writeSecureFile(keyPath, envelope('candidate-key'), { + durable: true, + shouldPublish + }) + ).toBe(false) + + expect(shouldPublish).toHaveBeenCalledTimes(1) + expect(mocks.rename).not.toHaveBeenCalled() + expect(realFs.existsSync(keyPath)).toBe(previous) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual( + previous ? ['zcode-plan-api-key.enc'] : [] + ) + if (previous) { + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('previous-key')) + } + } + ) + + it('cleans staged bytes when the publication predicate throws', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + + expect(() => + writer.writeSecureFile(keyPath, envelope('candidate-key'), { + shouldPublish: () => { + throw new Error('Synthetic ownership read failure') + } + }) + ).toThrow('Synthetic ownership read failure') + + expect(mocks.rename).not.toHaveBeenCalled() + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('previous-key')) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) + + it('retains best-effort restriction outcomes when guarded publication is allowed', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + const shouldPublish = vi.fn(() => true) + + expect( + writer.writeSecureFile(keyPath, envelope('candidate-key'), { + durable: true, + shouldPublish + }) + ).toBe(false) + + expect(shouldPublish).toHaveBeenCalledTimes(1) + expect(mocks.rename).toHaveBeenCalledTimes(1) + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('candidate-key')) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) + + it('checks publication immediately after staged hardening and before rename', () => { + const operations: string[] = [] + mocks.restrict.mockImplementation((path) => { + operations.push(path.endsWith('.tmp') ? 'staged restriction' : 'published restriction') + return true + }) + mocks.rename.mockImplementation((from, to) => { + operations.push('rename') + realFs.renameSync(from, to) + }) + + expect( + writer.writeSecureFile(keyPath, envelope('candidate-key'), { + shouldPublish: () => { + operations.push('ownership check') + return true + } + }) + ).toBe(true) + + expect(operations).toEqual([ + 'staged restriction', + 'ownership check', + 'rename', + 'published restriction' + ]) + }) + + it('cleans guarded staging bytes if rename throws without deleting the original target', () => { + realFs.writeFileSync(keyPath, envelope('previous-key')) + mocks.rename.mockImplementation(() => { + throw new Error('Synthetic guarded rename failure') + }) + + expect(() => + writer.writeSecureFile(keyPath, envelope('candidate-key'), { + shouldPublish: () => true + }) + ).toThrow('Synthetic guarded rename failure') + + expect(realFs.readFileSync(keyPath, 'utf8')).toBe(envelope('previous-key')) + expect(realFs.readdirSync(join(mocks.home, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) +}) diff --git a/src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts b/src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts new file mode 100644 index 000000000000..dca5e914eb0c --- /dev/null +++ b/src/main/zcode/zcode-plan-api-key-store-publication-race.test.ts @@ -0,0 +1,160 @@ +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + renameSync, + rmSync, + writeFileSync +} from 'node:fs' +import { homedir, tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as SecureFile from '../../shared/secure-file' +import type * as ZcodePlanApiKeyStore from './zcode-plan-api-key-store' + +const restriction = vi.hoisted(() => vi.fn((_path: string, _directory: boolean) => false)) + +vi.mock('electron', () => ({ safeStorage: { isEncryptionAvailable: () => false } })) +vi.mock('../../shared/secure-path-windows-acl', () => ({ + restrictWindowsPathSync: restriction, + bestEffortRestrictWindowsPath: ( + _path: string, + _directory: boolean, + settled: (restricted: boolean) => void + ) => settled(false), + resetSecureFileWindowsUserSidForTests: vi.fn() +})) + +const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')! +const envelope = (key: string): string => + `orca-zcode-plan-api-key:v1:plaintext:${Buffer.from(key).toString('base64')}` +const previousEnvelope = envelope('previous-key') +const competingEnvelope = envelope('competing-key') +let caseHome: string +let keyPath: string +let store: typeof ZcodePlanApiKeyStore +let writer: typeof SecureFile + +beforeEach(async () => { + caseHome = mkdtempSync(join(tmpdir(), 'orca-rollback-publication-')) + vi.stubEnv('HOME', caseHome) + vi.stubEnv('USERPROFILE', caseHome) + keyPath = join(caseHome, '.orca', 'zcode-plan-api-key.enc') + mkdirSync(join(caseHome, '.orca')) + restriction.mockReset().mockReturnValue(false) + vi.spyOn(console, 'warn').mockImplementation(() => undefined) + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + vi.resetModules() + expect(homedir()).toBe(caseHome) + store = await import('./zcode-plan-api-key-store') + writer = await import('../../shared/secure-file') + expect(vi.isMockFunction(writer.writeSecureFile)).toBe(false) + expect(vi.isMockFunction(writeFileSync)).toBe(false) + expect(vi.isMockFunction(renameSync)).toBe(false) +}) + +afterEach(() => { + Object.defineProperty(process, 'platform', platformDescriptor) + vi.unstubAllEnvs() + vi.restoreAllMocks() + rmSync(caseHome, { recursive: true, force: true }) +}) + +function publishDuringRollbackStaging(outcome: boolean | 'throws'): void { + let stagedRestrictions = 0 + restriction.mockImplementation((targetPath) => { + if (targetPath.endsWith('.tmp') && ++stagedRestrictions === 2) { + const competingPath = join(caseHome, '.orca', 'competing-envelope') + writeFileSync(competingPath, competingEnvelope) + renameSync(competingPath, keyPath) + if (outcome === 'throws') { + throw new Error('Synthetic rollback staged ACL failure') + } + return outcome + } + return false + }) +} + +function refuseReplacement(): void { + expect(() => store.saveZcodePlanApiKey('attempted-key')).toThrow( + 'could not be stored securely on this device' + ) +} + +describe('rollback publication with real filesystem writes and renames', () => { + it.each([false, true, 'throws'] as const)( + 'preserves the competing publication when the second staged ACL call returns %s', + (outcome) => { + writeFileSync(keyPath, previousEnvelope) + publishDuringRollbackStaging(outcome) + + refuseReplacement() + + expect(readFileSync(keyPath, 'utf8')).toBe(competingEnvelope) + expect(readFileSync(keyPath, 'utf8')).not.toBe(envelope('attempted-key')) + expect(store.readZcodePlanApiKey()).toBe('competing-key') + expect(readdirSync(join(caseHome, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + } + ) + + it('restores the previous envelope when the attempted publication is still owned', () => { + writeFileSync(keyPath, previousEnvelope) + + refuseReplacement() + + expect(readFileSync(keyPath, 'utf8')).toBe(previousEnvelope) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + expect(readdirSync(join(caseHome, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) + + it('removes the owned rejected plaintext when no previous envelope exists', () => { + refuseReplacement() + + expect(existsSync(keyPath)).toBe(false) + expect(store.readZcodePlanApiKey()).toBeNull() + expect(readdirSync(join(caseHome, '.orca'))).toEqual([]) + }) + + it.each([false, true])( + 'retains the prior target state when initial staged hardening throws: previous %s', + (previous) => { + if (previous) { + writeFileSync(keyPath, previousEnvelope) + } + restriction.mockImplementation((targetPath) => { + if (targetPath.endsWith('.tmp')) { + throw new Error('Synthetic initial staged ACL failure') + } + return false + }) + + expect(() => store.saveZcodePlanApiKey('attempted-key')).toThrow( + 'Synthetic initial staged ACL failure' + ) + + expect(existsSync(keyPath)).toBe(previous) + expect(store.readZcodePlanApiKey()).toBe(previous ? 'previous-key' : null) + expect(readdirSync(join(caseHome, '.orca'))).toEqual( + previous ? ['zcode-plan-api-key.enc'] : [] + ) + if (previous) { + expect(readFileSync(keyPath, 'utf8')).toBe(previousEnvelope) + } + } + ) + + it('keeps the cached prior key after rejecting a replacement during competing publication', () => { + writeFileSync(keyPath, previousEnvelope) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + publishDuringRollbackStaging(false) + + refuseReplacement() + + expect(readFileSync(keyPath, 'utf8')).toBe(competingEnvelope) + expect(store.readZcodePlanApiKey()).toBe('previous-key') + expect(readdirSync(join(caseHome, '.orca'))).toEqual(['zcode-plan-api-key.enc']) + }) +}) diff --git a/src/main/zcode/zcode-plan-api-key-store.test.ts b/src/main/zcode/zcode-plan-api-key-store.test.ts index ed53cd28e7b3..7e9f6f6b7b69 100644 --- a/src/main/zcode/zcode-plan-api-key-store.test.ts +++ b/src/main/zcode/zcode-plan-api-key-store.test.ts @@ -141,6 +141,7 @@ describe('zcode-plan-api-key-store', () => { safeStorageMock.isEncryptionAvailable.mockReturnValue(false) writeSecureFileMock.mockReturnValue(false) existsSyncMock.mockReturnValue(false) + readFileSyncMock.mockReturnValue(Buffer.from(envelope('plaintext', 'glm-secret'))) const store = await loadStore() expect(() => store.saveZcodePlanApiKey('glm-secret')).toThrow( @@ -155,7 +156,10 @@ describe('zcode-plan-api-key-store', () => { safeStorageMock.isEncryptionAvailable.mockReturnValue(false) const previous = Buffer.from(envelope('encrypted', 'old-key')) existsSyncMock.mockReturnValue(true) - readFileSyncMock.mockReturnValue(previous) + readFileSyncMock + .mockReturnValueOnce(previous) + .mockReturnValueOnce(Buffer.from(envelope('plaintext', 'new-key'))) + .mockReturnValue(previous) writeSecureFileMock.mockReturnValueOnce(false).mockReturnValueOnce(true) const store = await loadStore() @@ -165,7 +169,67 @@ describe('zcode-plan-api-key-store', () => { // First write publishes the unrestricted replacement; the second restores // the previous envelope so the user's old key survives the failed replace. expect(writeSecureFileMock).toHaveBeenCalledTimes(2) - expect(writeSecureFileMock).toHaveBeenLastCalledWith(storePath, previous.toString('utf8')) + expect(writeSecureFileMock).toHaveBeenLastCalledWith(storePath, previous.toString('utf8'), { + shouldPublish: expect.any(Function) + }) + expect(rmSyncMock).not.toHaveBeenCalledWith(storePath, expect.anything()) + warn.mockRestore() + }) + + it('removes a rejected plaintext replacement when restoring the previous key fails', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + safeStorageMock.isEncryptionAvailable.mockReturnValue(false) + const previous = Buffer.from(envelope('encrypted', 'old-key')) + const attempted = envelope('plaintext', 'new-key') + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValueOnce(previous).mockReturnValue(Buffer.from(attempted)) + writeSecureFileMock.mockReturnValueOnce(false).mockImplementationOnce(() => { + throw new Error('restore failed before publish') + }) + const store = await loadStore() + + expect(() => store.saveZcodePlanApiKey('new-key')).toThrow( + 'could not be stored securely on this device' + ) + expect(rmSyncMock).toHaveBeenCalledWith(storePath, { force: true }) + warn.mockRestore() + }) + + it('keeps a restored key when rollback fails after the previous envelope is published', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + safeStorageMock.isEncryptionAvailable.mockReturnValue(false) + const previous = Buffer.from(envelope('encrypted', 'old-key')) + existsSyncMock.mockReturnValue(true) + readFileSyncMock + .mockReturnValueOnce(previous) + .mockReturnValueOnce(Buffer.from(envelope('plaintext', 'new-key'))) + .mockReturnValue(previous) + writeSecureFileMock.mockReturnValueOnce(false).mockImplementationOnce(() => { + throw new Error('restore failed after publish') + }) + const store = await loadStore() + + expect(() => store.saveZcodePlanApiKey('new-key')).toThrow( + 'could not be stored securely on this device' + ) + expect(rmSyncMock).not.toHaveBeenCalledWith(storePath, expect.anything()) + warn.mockRestore() + }) + + it('keeps an identical plaintext key when restore fails after publishing it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + safeStorageMock.isEncryptionAvailable.mockReturnValue(false) + const sameKey = Buffer.from(envelope('plaintext', 'same-key')) + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValue(sameKey) + writeSecureFileMock.mockReturnValueOnce(false).mockImplementationOnce(() => { + throw new Error('restore failed after publish') + }) + const store = await loadStore() + + expect(() => store.saveZcodePlanApiKey('same-key')).toThrow( + 'could not be stored securely on this device' + ) expect(rmSyncMock).not.toHaveBeenCalledWith(storePath, expect.anything()) warn.mockRestore() }) diff --git a/src/main/zcode/zcode-plan-api-key-store.ts b/src/main/zcode/zcode-plan-api-key-store.ts index 45c4ad315ac6..7aad68366610 100644 --- a/src/main/zcode/zcode-plan-api-key-store.ts +++ b/src/main/zcode/zcode-plan-api-key-store.ts @@ -84,6 +84,37 @@ export function getZcodePlanApiKeyProtection(): SecretAtRestProtection | null { } } +function rollbackPlaintextEnvelope( + keyPath: string, + attemptedEnvelope: string, + previousEnvelope: Buffer | null +): void { + const attempted = Buffer.from(attemptedEnvelope, 'utf8') + try { + if (!readFileSync(keyPath).equals(attempted)) { + return + } + if (previousEnvelope) { + try { + writeSecureFile(keyPath, previousEnvelope.toString('utf8'), { + shouldPublish: () => readFileSync(keyPath).equals(attempted) + }) + } catch { + // A failed restore may already have published the previous envelope. + } + } + const currentEnvelope = readFileSync(keyPath) + if ( + currentEnvelope.equals(attempted) && + (!previousEnvelope || !currentEnvelope.equals(previousEnvelope)) + ) { + rmSync(keyPath, { force: true }) + } + } catch { + // Unreadable current bytes do not establish ownership of the rejected write. + } +} + export function saveZcodePlanApiKey(key: string): void { const trimmed = key.trim() if (!trimmed) { @@ -115,23 +146,11 @@ export function saveZcodePlanApiKey(key: string): void { previousEnvelope = null } } - const wroteRestricted = writeSecureFile( - keyPath, - encodeApiKeyEnvelope('plaintext', Buffer.from(trimmed, 'utf8')) - ) + const attemptedEnvelope = encodeApiKeyEnvelope('plaintext', Buffer.from(trimmed, 'utf8')) + const wroteRestricted = writeSecureFile(keyPath, attemptedEnvelope) // Why: an unrestricted plaintext credential must never be reported as saved. if (!wroteRestricted) { - if (!previousEnvelope) { - rmSync(keyPath, { force: true }) - } else { - try { - writeSecureFile(keyPath, previousEnvelope.toString('utf8')) - } catch { - // Why: restriction is failing device-wide; the restored bytes keep the - // previous credential available instead of deleting it, and the thrown - // save error still tells the user the store is not secure. - } - } + rollbackPlaintextEnvelope(keyPath, attemptedEnvelope, previousEnvelope) throw new Error('GLM Coding Plan API key could not be stored securely on this device') } cachedZcodePlanApiKey = trimmed diff --git a/src/renderer/src/components/automations/use-automations-page-setup-state.ts b/src/renderer/src/components/automations/use-automations-page-setup-state.ts index ae0939085fe3..5ada1d683d28 100644 --- a/src/renderer/src/components/automations/use-automations-page-setup-state.ts +++ b/src/renderer/src/components/automations/use-automations-page-setup-state.ts @@ -150,8 +150,7 @@ export function useAutomationsPageSetupState({ automationYamlHooksByRepoKey, getAutomationHooksCacheKey, repos, - setAutomationYamlHooksByRepoKey, - settings + setAutomationYamlHooksByRepoKey ] ) const getDraftSetupDecisionDefaultSignature = useCallback( diff --git a/src/renderer/src/components/sidebar/folder-workspace-composer-submit-fixtures.ts b/src/renderer/src/components/sidebar/folder-workspace-composer-submit-fixtures.ts new file mode 100644 index 000000000000..303445980ab1 --- /dev/null +++ b/src/renderer/src/components/sidebar/folder-workspace-composer-submit-fixtures.ts @@ -0,0 +1,36 @@ +import type { FolderWorkspace } from '../../../../shared/folder-workspace-types' +import type { ProjectGroup } from '../../../../shared/project-group-types' + +export function makeProjectGroup(): ProjectGroup { + return { + id: 'group-1', + name: 'Platform', + parentPath: '/repo/platform', + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } +} + +export function makeFolderWorkspace(overrides: Partial = {}): FolderWorkspace { + return { + id: 'folder-workspace-1', + projectGroupId: 'group-1', + name: 'hi', + folderPath: '/repo/platform/hi', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1, + ...overrides + } +} diff --git a/src/renderer/src/components/sidebar/folder-workspace-composer-submit-host-platform.test.ts b/src/renderer/src/components/sidebar/folder-workspace-composer-submit-host-platform.test.ts index f1fcb5e6f008..95a01c86f40b 100644 --- a/src/renderer/src/components/sidebar/folder-workspace-composer-submit-host-platform.test.ts +++ b/src/renderer/src/components/sidebar/folder-workspace-composer-submit-host-platform.test.ts @@ -1,8 +1,7 @@ // @vitest-environment happy-dom import { afterEach, describe, expect, it, vi } from 'vitest' -import type { FolderWorkspace } from '../../../../shared/folder-workspace-types' -import type { ProjectGroup } from '../../../../shared/project-group-types' +import { makeProjectGroup, makeFolderWorkspace } from './folder-workspace-composer-submit-fixtures' const mocks = vi.hoisted(() => ({ activateAndRevealFolderWorkspace: vi.fn() })) @@ -14,39 +13,6 @@ vi.mock('@/lib/worktree-activation', async (importOriginal) => { import { useAppStore } from '@/store' import { submitFolderWorkspaceCreate } from './folder-workspace-composer-submit' -function makeProjectGroup(): ProjectGroup { - return { - id: 'group-1', - name: 'Platform', - parentPath: '/repo/platform', - parentGroupId: null, - createdFrom: 'folder-scan', - tabOrder: 0, - isCollapsed: false, - color: null, - createdAt: 1, - updatedAt: 1 - } -} - -function makeFolderWorkspace(): FolderWorkspace { - return { - id: 'folder-workspace-1', - projectGroupId: 'group-1', - name: 'hi', - folderPath: '/repo/platform/hi', - linkedTask: null, - comment: '', - isArchived: false, - isUnread: false, - isPinned: false, - sortOrder: 0, - lastActivityAt: 1, - createdAt: 1, - updatedAt: 1 - } -} - // Why: a folder group on an Orca server runs its agents there, so the server's OS decides quoting. describe('submitFolderWorkspaceCreate on an Orca server', () => { afterEach(() => { diff --git a/src/renderer/src/components/sidebar/folder-workspace-composer-submit.test.ts b/src/renderer/src/components/sidebar/folder-workspace-composer-submit.test.ts index 85d5f2020be0..0964e076f87a 100644 --- a/src/renderer/src/components/sidebar/folder-workspace-composer-submit.test.ts +++ b/src/renderer/src/components/sidebar/folder-workspace-composer-submit.test.ts @@ -1,8 +1,7 @@ // @vitest-environment happy-dom import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import type { FolderWorkspace } from '../../../../shared/folder-workspace-types' -import type { ProjectGroup } from '../../../../shared/project-group-types' +import { makeProjectGroup, makeFolderWorkspace } from './folder-workspace-composer-submit-fixtures' import { folderWorkspaceKey } from '../../../../shared/workspace-scope' import type * as NewWorkspaceModule from '@/lib/new-workspace' @@ -30,40 +29,6 @@ import { submitFolderWorkspaceCreate } from './folder-workspace-composer-submit' -function makeProjectGroup(): ProjectGroup { - return { - id: 'group-1', - name: 'Platform', - parentPath: '/repo/platform', - parentGroupId: null, - createdFrom: 'folder-scan', - tabOrder: 0, - isCollapsed: false, - color: null, - createdAt: 1, - updatedAt: 1 - } -} - -function makeFolderWorkspace(overrides: Partial = {}): FolderWorkspace { - return { - id: 'folder-workspace-1', - projectGroupId: 'group-1', - name: 'hi', - folderPath: '/repo/platform/hi', - linkedTask: null, - comment: '', - isArchived: false, - isUnread: false, - isPinned: false, - sortOrder: 0, - lastActivityAt: 1, - createdAt: 1, - updatedAt: 1, - ...overrides - } -} - describe('submitFolderWorkspaceCreate', () => { it.each(['347', '002'])('keeps manual folder name %s with its linked source', async (name) => { const linkedWorkItem = { diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index c04ea9afe5b0..172436dcc5b1 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -116,6 +116,8 @@ export function writeDurableSecureJsonFile(targetPath: string, value: unknown): * legitimately on FAT32, network paths and restricted tokens, and must not break a write — but * the outcome is now reported rather than assumed, so a caller storing a credential can react. * + * An optional publication predicate runs after staged hardening; false discards the staged file. + * * The return value covers the *file* only. The parent directory is hardened fire-and-forget — on * Windows that lane is async and answers `pending` regardless — so a `true` here says nothing * about the directory's ACL. @@ -123,7 +125,7 @@ export function writeDurableSecureJsonFile(targetPath: string, value: unknown): export function writeSecureFile( targetPath: string, contents: string, - options: { durable?: boolean } = {} + options: { durable?: boolean; shouldPublish?: () => boolean } = {} ): boolean { const dir = dirname(targetPath) if (!existsSync(dir)) { @@ -143,6 +145,11 @@ export function writeSecureFile( } // Why: writeFileSync mode is a no-op on Windows, so restrict the credential's ACL synchronously before the rename publishes it under inherited ACLs. const stagedOutcome = applySecurePathRestriction(tmpFile, false, process.platform, true) + // Recheck after hardening; the check and rename are separate, non-atomic operations. + if (options.shouldPublish && !options.shouldPublish()) { + rmSync(tmpFile, { force: true }) + return false + } renameSync(tmpFile, targetPath) // Why: these hold auth credentials, so the published path must stay current-user only; cache only on confirmed success so failures retry. // The staged file's protected DACL survives the rename, so this pass usually just verifies it.