diff --git a/src/process/extensions/sandbox/permissions.ts b/src/process/extensions/sandbox/permissions.ts index c6171db90..e6df8e838 100644 --- a/src/process/extensions/sandbox/permissions.ts +++ b/src/process/extensions/sandbox/permissions.ts @@ -78,6 +78,17 @@ export interface PermissionSummary { granted: boolean; } +export function hasSandboxStoragePermission(permissions?: ExtPermissions): boolean { + return permissions?.storage === true; +} + +export function getSandboxPermissionDeniedError(method: string, permissions?: ExtPermissions): string | null { + if (method.startsWith('storage.') && !hasSandboxStoragePermission(permissions)) { + return 'Permission denied: storage access requires "storage: true" in manifest'; + } + return null; +} + // ============ Permission Analysis ============ /** diff --git a/src/process/extensions/sandbox/sandbox.ts b/src/process/extensions/sandbox/sandbox.ts index 8050a50be..c6e602b1a 100644 --- a/src/process/extensions/sandbox/sandbox.ts +++ b/src/process/extensions/sandbox/sandbox.ts @@ -4,9 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { Worker, type MessagePort } from 'worker_threads'; +import { Worker } from 'worker_threads'; import * as path from 'path'; -import type { ExtPermissions } from './permissions'; +import { getSandboxPermissionDeniedError, type ExtPermissions } from './permissions'; import { extensionEventBus } from '../lifecycle/ExtensionEventBus'; /** @@ -282,6 +282,16 @@ export class SandboxHost { * Handle an api-call from the Worker and send back an api-response. */ private handleWorkerApiCall(id: string, method: string, args: unknown[]): void { + const permissionError = getSandboxPermissionDeniedError(method, this.options.permissions); + if (permissionError) { + this.worker?.postMessage({ + type: 'api-response', + id, + error: permissionError, + } satisfies SandboxMessage); + return; + } + const handler = this.options.apiHandlers?.[method]; if (!handler) { this.worker?.postMessage({ diff --git a/src/process/extensions/sandbox/sandboxWorker.ts b/src/process/extensions/sandbox/sandboxWorker.ts index 28db1165d..2e892d6f2 100644 --- a/src/process/extensions/sandbox/sandboxWorker.ts +++ b/src/process/extensions/sandbox/sandboxWorker.ts @@ -17,17 +17,19 @@ * - Extension code runs with full Worker Thread privileges (Node.js built-ins accessible) * - Electron main-process APIs are not directly accessible (different process/thread) * - The `aion` proxy provides a structured communication channel to the main thread - * - Declared permissions in the manifest are NOT enforced at runtime — they are - * informational only and used for UI display purposes + * - Declared permissions in the manifest are partially enforced at runtime: + * storage access is gated by the storage permission flag on both the worker + * and host sides. Other permissions remain informational only. * - * TODO: Enforce declared permissions at runtime (e.g. via vm.runInNewContext + - * custom require proxy, or Node.js --experimental-permission flag) to prevent - * extensions from accessing undeclared Node.js APIs. + * TODO: Enforce remaining declared permissions at runtime (e.g. via + * vm.runInNewContext + custom require proxy, or Node.js --experimental-permission + * flag) to prevent extensions from accessing undeclared Node.js APIs. */ import { parentPort, workerData } from 'worker_threads'; import * as path from 'path'; import type { SandboxMessage } from './sandbox'; +import { hasSandboxStoragePermission } from './permissions'; if (!parentPort) { throw new Error('This script must be run as a Worker Thread'); @@ -78,16 +80,19 @@ const aionProxy = { port.postMessage({ type: 'event', name: `ext:${eventName}`, payload } satisfies SandboxMessage); }, - /** Storage API (if permission granted) */ - storage: { - get: async (key: string): Promise => callMainThread('storage.get', [key]), - set: async (key: string, value: unknown): Promise => { - await callMainThread('storage.set', [key, value]); - }, - delete: async (key: string): Promise => { - await callMainThread('storage.delete', [key]); - }, - }, + ...(hasSandboxStoragePermission(config.permissions) + ? { + storage: { + get: async (key: string): Promise => callMainThread('storage.get', [key]), + set: async (key: string, value: unknown): Promise => { + await callMainThread('storage.set', [key, value]); + }, + delete: async (key: string): Promise => { + await callMainThread('storage.delete', [key]); + }, + }, + } + : {}), }; let callIdCounter = 0; diff --git a/tests/unit/extensions/sandboxHost.test.ts b/tests/unit/extensions/sandboxHost.test.ts index 1c6208427..1026cfd1d 100644 --- a/tests/unit/extensions/sandboxHost.test.ts +++ b/tests/unit/extensions/sandboxHost.test.ts @@ -40,9 +40,36 @@ function createHost(overrides: Partial describe('extensions/SandboxHost — handleMessage', () => { describe('api-call routing (Bug #4 fix)', () => { + it('should reject storage api-call before routing when storage permission is missing', () => { + const handler = vi.fn(); + const host = createHost({ + apiHandlers: { 'storage.get': handler }, + permissions: undefined, + }); + const mock = createMockWorker(); + + (host as unknown as { worker: unknown }).worker = mock.instance; + (host as unknown as { _running: boolean })._running = true; + + const msg: SandboxMessage = { type: 'api-call', id: 'w-0', method: 'storage.get', args: ['myKey'] }; + (host as unknown as { handleMessage: (m: SandboxMessage) => void }).handleMessage(msg); + + expect(handler).not.toHaveBeenCalled(); + expect(mock.posted).toEqual([ + { + type: 'api-response', + id: 'w-0', + error: 'Permission denied: storage access requires "storage: true" in manifest', + }, + ]); + }); + it('should route Worker api-call to registered apiHandler and respond with result', () => { const handler = vi.fn().mockReturnValue('hello'); - const host = createHost({ apiHandlers: { 'storage.get': handler } }); + const host = createHost({ + apiHandlers: { 'storage.get': handler }, + permissions: { storage: true, events: true }, + }); const mock = createMockWorker(); // Inject mock worker @@ -60,7 +87,10 @@ describe('extensions/SandboxHost — handleMessage', () => { it('should handle async apiHandler and respond with resolved value', async () => { const handler = vi.fn().mockResolvedValue({ items: [1, 2] }); - const host = createHost({ apiHandlers: { 'storage.get': handler } }); + const host = createHost({ + apiHandlers: { 'storage.get': handler }, + permissions: { storage: true, events: true }, + }); const mock = createMockWorker(); (host as unknown as { worker: unknown }).worker = mock.instance; (host as unknown as { _running: boolean })._running = true; @@ -79,7 +109,10 @@ describe('extensions/SandboxHost — handleMessage', () => { const handler = vi.fn().mockImplementation(() => { throw new Error('boom'); }); - const host = createHost({ apiHandlers: { 'storage.set': handler } }); + const host = createHost({ + apiHandlers: { 'storage.set': handler }, + permissions: { storage: true, events: true }, + }); const mock = createMockWorker(); (host as unknown as { worker: unknown }).worker = mock.instance; (host as unknown as { _running: boolean })._running = true; @@ -93,7 +126,10 @@ describe('extensions/SandboxHost — handleMessage', () => { it('should respond with error when async apiHandler rejects', async () => { const handler = vi.fn().mockRejectedValue(new Error('async boom')); - const host = createHost({ apiHandlers: { 'storage.delete': handler } }); + const host = createHost({ + apiHandlers: { 'storage.delete': handler }, + permissions: { storage: true, events: true }, + }); const mock = createMockWorker(); (host as unknown as { worker: unknown }).worker = mock.instance; (host as unknown as { _running: boolean })._running = true; @@ -125,7 +161,9 @@ describe('extensions/SandboxHost — handleMessage', () => { }); it('should respond with error when apiHandlers option is not provided', () => { - const host = createHost(); // no apiHandlers + const host = createHost({ + permissions: { storage: true, events: true }, + }); // no apiHandlers const mock = createMockWorker(); (host as unknown as { worker: unknown }).worker = mock.instance; (host as unknown as { _running: boolean })._running = true; diff --git a/tests/unit/extensions/sandboxPermission.test.ts b/tests/unit/extensions/sandboxPermission.test.ts new file mode 100644 index 000000000..2d9340a3d --- /dev/null +++ b/tests/unit/extensions/sandboxPermission.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from 'vitest'; +import { + getSandboxPermissionDeniedError, + hasSandboxStoragePermission, +} from '../../../src/process/extensions/sandbox/permissions'; + +describe('extensions/sandbox permissions', () => { + it('treats omitted permissions as no storage access', () => { + expect(hasSandboxStoragePermission(undefined)).toBe(false); + }); + + it('treats storage permission false as no storage access', () => { + expect(hasSandboxStoragePermission({ storage: false, events: true })).toBe(false); + }); + + it('treats storage permission true as storage access enabled', () => { + expect(hasSandboxStoragePermission({ storage: true, events: true })).toBe(true); + }); + + it('returns a permission error for storage api calls without storage permission', () => { + expect(getSandboxPermissionDeniedError('storage.get', undefined)).toBe( + 'Permission denied: storage access requires "storage: true" in manifest' + ); + }); + + it('does not return a permission error for non-storage api calls without storage permission', () => { + expect(getSandboxPermissionDeniedError('custom.method', undefined)).toBeNull(); + }); + + it('does not return a permission error for storage api calls when storage permission is granted', () => { + expect(getSandboxPermissionDeniedError('storage.get', { storage: true, events: true })).toBeNull(); + }); +});