From b2b3e6d44b31a61dc2e8eae74023dd460c9c17ff Mon Sep 17 00:00:00 2001 From: Alok Swamy Date: Thu, 6 Aug 2026 17:40:27 -0400 Subject: [PATCH] Require VS Code consent for custom theme checks --- .changeset/bright-themes-ask.md | 8 ++ .../src/config/load-config-description.ts | 32 ++++-- .../src/config/load-config.spec.ts | 93 ++++++++++++++++ .../src/config/load-config.ts | 5 +- packages/theme-check-node/src/config/types.ts | 19 ++++ .../theme-language-server-common/src/types.ts | 17 +++ .../src/dependencies.ts | 17 ++- .../theme-language-server-node/src/index.ts | 19 +++- .../src/node/customCheckConsent.spec.ts | 102 ++++++++++++++++++ .../src/node/customCheckConsent.ts | 100 +++++++++++++++++ .../vscode-extension/src/node/extension.ts | 4 + packages/vscode-extension/src/node/server.ts | 12 ++- 12 files changed, 411 insertions(+), 17 deletions(-) create mode 100644 .changeset/bright-themes-ask.md create mode 100644 packages/vscode-extension/src/node/customCheckConsent.spec.ts create mode 100644 packages/vscode-extension/src/node/customCheckConsent.ts diff --git a/.changeset/bright-themes-ask.md b/.changeset/bright-themes-ask.md new file mode 100644 index 000000000..04308ebc7 --- /dev/null +++ b/.changeset/bright-themes-ask.md @@ -0,0 +1,8 @@ +--- +'@shopify/theme-check-node': minor +'@shopify/theme-language-server-common': minor +'@shopify/theme-language-server-node': minor +'theme-check-vscode': minor +--- + +Ask for explicit VS Code consent before executing custom Theme Check code selected by a theme configuration. diff --git a/packages/theme-check-node/src/config/load-config-description.ts b/packages/theme-check-node/src/config/load-config-description.ts index 15385d129..68a75dd23 100644 --- a/packages/theme-check-node/src/config/load-config-description.ts +++ b/packages/theme-check-node/src/config/load-config-description.ts @@ -10,7 +10,7 @@ import { fileExists } from '../file-utils'; import { AbsolutePath } from '../temp'; import { thisNodeModuleRoot } from './installation-location'; import { findThirdPartyChecks, loadThirdPartyChecks } from './load-third-party-checks'; -import { ConfigDescription } from './types'; +import { ConfigDescription, CustomCheckCandidate, LoadConfigOptions } from './types'; import { URI, Utils } from 'vscode-uri'; const flatten = (arrs: T[][]): T[] => arrs.flat(); @@ -24,16 +24,27 @@ const flatten = (arrs: T[][]): T[] => arrs.flat(); export async function loadConfigDescription( configDescription: ConfigDescription, root: AbsolutePath, + options: LoadConfigOptions = {}, ): Promise { const nodeModuleRoot = await findNodeModuleRoot(root); - const thirdPartyChecksPaths = await Promise.all([ + const [globalThirdPartyChecksPaths, workspaceThirdPartyChecksPaths] = await Promise.all([ findThirdPartyChecks(thisNodeModuleRoot()), // global checks findThirdPartyChecks(nodeModuleRoot), - ]).then(flatten); - const thirdPartyChecks = loadThirdPartyChecks([ - ...configDescription.require, - ...thirdPartyChecksPaths, ]); + const customCheckCandidates = uniqueCandidates([ + ...configDescription.require.map((path) => ({ source: 'require' as const, path })), + ...flatten([globalThirdPartyChecksPaths, workspaceThirdPartyChecksPaths]).map((path) => ({ + source: 'discovery' as const, + path, + })), + ]); + const customChecksAuthorized = + customCheckCandidates.length === 0 || + !options.authorizeCustomChecks || + (await options.authorizeCustomChecks({ root, candidates: customCheckCandidates })); + const thirdPartyChecks = customChecksAuthorized + ? loadThirdPartyChecks(customCheckCandidates.map(({ path }) => path)) + : []; const checks: CheckDefinition[] = allChecks .concat(thirdPartyChecks) .filter(isEnabledBy(configDescription)); @@ -48,6 +59,15 @@ export async function loadConfigDescription( }; } +function uniqueCandidates(candidates: CustomCheckCandidate[]): CustomCheckCandidate[] { + const seen = new Set(); + return candidates.filter(({ path }) => { + if (seen.has(path)) return false; + seen.add(path); + return true; + }); +} + /** * @param root - absolute path of the config file * @param pathLike - resolved textual value of the `root` property from the config files diff --git a/packages/theme-check-node/src/config/load-config.spec.ts b/packages/theme-check-node/src/config/load-config.spec.ts index bcd473bbd..8c4d9d44f 100644 --- a/packages/theme-check-node/src/config/load-config.spec.ts +++ b/packages/theme-check-node/src/config/load-config.spec.ts @@ -67,6 +67,15 @@ describe('Unit: loadConfig', () => { expect(config.ignore).to.include('src/**'); }); + it('does not request authorization when there are no custom checks', async () => { + const configPath = await createMockConfigFile(tempDir, `extends: theme-check:recommended`); + const authorizeCustomChecks = vi.fn().mockResolvedValue(false); + + await loadConfig(configPath, tempDir, { authorizeCustomChecks }); + + expect(authorizeCustomChecks).not.toHaveBeenCalled(); + }); + it('has no checks if it extends nothing', async () => { const configPath = await createMockConfigFile(tempDir, `extends: nothing`); const config = await loadConfig(configPath, tempDir); @@ -217,6 +226,90 @@ NodeModuleCheck: expect(nodeModuleCheck).to.exist; }); + it('does not execute a required extension before it is authorized', async () => { + const configPath = await createMockConfigFile( + tempDir, + ` +extends: nothing +require: './checks.js' +NodeModuleCheck: + enabled: true + `, + ); + const markerPath = path.join(tempDir, 'executed'); + await fs.writeFile( + path.join(tempDir, 'checks.js'), + `require('node:fs').writeFileSync(${JSON.stringify(markerPath)}, 'yes');\n${mockNodeModuleCheck}`, + ); + const authorizeCustomChecks = vi.fn().mockResolvedValue(false); + + const config = await loadConfig(configPath, tempDir, { authorizeCustomChecks }); + + expect(authorizeCustomChecks).toHaveBeenCalledWith({ + root: tempDir, + candidates: [{ source: 'require', path: path.join(tempDir, 'checks.js') }], + }); + expect(config.checks.find((check) => check.meta.code === 'NodeModuleCheck')).not.to.exist; + await expect(fs.stat(markerPath)).rejects.toThrow(); + }); + + it('asks for authorization for extensions inherited through extends', async () => { + await fs.writeFile( + path.join(tempDir, 'base.yml'), + ` +extends: nothing +require: './checks.js' + `, + ); + await fs.writeFile(path.join(tempDir, 'checks.js'), mockNodeModuleCheck); + const configPath = await createMockConfigFile(tempDir, `extends: './base.yml'`); + const authorizeCustomChecks = vi.fn().mockResolvedValue(false); + + await loadConfig(configPath, tempDir, { authorizeCustomChecks }); + + expect(authorizeCustomChecks).toHaveBeenCalledWith({ + root: tempDir, + candidates: [{ source: 'require', path: path.join(tempDir, 'checks.js') }], + }); + }); + + it('asks for authorization before loading automatically discovered extensions', async () => { + const configPath = path.resolve(__dirname, 'fixtures/node-module-rec.yml'); + const modulePath = await createMockNodeModule( + tempDir, + 'theme-check-node-example', + mockNodeModuleCheck, + ); + const authorizeCustomChecks = vi.fn().mockResolvedValue(false); + + const config = await loadConfig(configPath, tempDir, { authorizeCustomChecks }); + + expect(authorizeCustomChecks).toHaveBeenCalledWith({ + root: tempDir, + candidates: [{ source: 'discovery', path: modulePath }], + }); + expect(config.checks.find((check) => check.meta.code === 'NodeModuleCheck')).not.to.exist; + }); + + it('loads custom checks after they are authorized', async () => { + const configPath = await createMockConfigFile( + tempDir, + ` +extends: nothing +require: './checks.js' +NodeModuleCheck: + enabled: true + `, + ); + await fs.writeFile(path.join(tempDir, 'checks.js'), mockNodeModuleCheck); + + const config = await loadConfig(configPath, tempDir, { + authorizeCustomChecks: vi.fn().mockResolvedValue(true), + }); + + expect(config.checks.find((check) => check.meta.code === 'NodeModuleCheck')).to.exist; + }); + it('loads an aliased check properly', async () => { const configPath = await createMockConfigFile( tempDir, diff --git a/packages/theme-check-node/src/config/load-config.ts b/packages/theme-check-node/src/config/load-config.ts index 42361562f..01a9c7430 100644 --- a/packages/theme-check-node/src/config/load-config.ts +++ b/packages/theme-check-node/src/config/load-config.ts @@ -2,7 +2,7 @@ import { Config } from '@shopify/theme-check-common'; import { AbsolutePath } from '../temp'; import { loadConfigDescription } from './load-config-description'; import { resolveConfig } from './resolve'; -import { ModernIdentifier } from './types'; +import { LoadConfigOptions, ModernIdentifier } from './types'; import { validateConfig } from './validation'; import fs from 'fs/promises'; @@ -21,6 +21,7 @@ export async function loadConfig( configPath: AbsolutePath | ModernIdentifier | undefined, /** The root of the theme */ root: AbsolutePath, + options: LoadConfigOptions = {}, ): Promise { if (!root) throw new Error('loadConfig cannot be called without a root argument'); let defaultChecks = 'theme-check:recommended'; @@ -35,7 +36,7 @@ export async function loadConfig( } const configDescription = await resolveConfig(configPath ?? defaultChecks, true); - const config = await loadConfigDescription(configDescription, root); + const config = await loadConfigDescription(configDescription, root, options); validateConfig(config); return config; } diff --git a/packages/theme-check-node/src/config/types.ts b/packages/theme-check-node/src/config/types.ts index 1bb8fcf06..1e9607ea0 100644 --- a/packages/theme-check-node/src/config/types.ts +++ b/packages/theme-check-node/src/config/types.ts @@ -30,6 +30,25 @@ export type ConfigDescription = Omit & { context: Mode; }; +export interface CustomCheckCandidate { + source: 'require' | 'discovery'; + path: string; +} + +export interface CustomCheckAuthorizationRequest { + root: string; + candidates: CustomCheckCandidate[]; +} + +export interface LoadConfigOptions { + /** + * Called after custom checks have been discovered, but before any of their + * JavaScript is loaded. When omitted, custom checks retain their existing + * behaviour and are loaded without an additional authorization step. + */ + authorizeCustomChecks?: (request: CustomCheckAuthorizationRequest) => Promise; +} + export const ModernIdentifiers = [ 'theme-check:nothing', 'theme-check:recommended', diff --git a/packages/theme-language-server-common/src/types.ts b/packages/theme-language-server-common/src/types.ts index 57acb1420..9832255ba 100644 --- a/packages/theme-language-server-common/src/types.ts +++ b/packages/theme-language-server-common/src/types.ts @@ -148,6 +148,23 @@ export namespace ThemeGraphDidUpdateNotification { } } +export namespace CustomCheckPermissionRequest { + export const method = 'themeCheck/requestCustomCheckPermission'; + export const type = new rpc.RequestType(method); + + export interface Candidate { + source: 'require' | 'discovery'; + path: string; + } + + export interface Params { + root: string; + candidates: Candidate[]; + } + + export type Response = boolean; +} + export type AugmentedLocationWithExistence = { uri: string; range: undefined; diff --git a/packages/theme-language-server-node/src/dependencies.ts b/packages/theme-language-server-node/src/dependencies.ts index 050b97c8e..cd8dba0ee 100644 --- a/packages/theme-language-server-node/src/dependencies.ts +++ b/packages/theme-language-server-node/src/dependencies.ts @@ -1,6 +1,7 @@ import { AbstractFileSystem, Config, + LoadConfigOptions, findRoot, loadConfig as nodeLoadConfig, makeFileExists, @@ -28,7 +29,11 @@ const hasThemeAppExtensionConfig = async (rootUri: string, fs: AbstractFileSyste return files.length > 0; }; -export const loadConfig: Dependencies['loadConfig'] = async function loadConfig(uriString, fs) { +export async function loadConfig( + uriString: string, + fs: AbstractFileSystem, + options: LoadConfigOptions = {}, +): Promise { const fileUri = path.normalize(uriString); const fileExists = makeFileExists(fs); const rootUriString = await findRoot(fileUri, fileExists); @@ -47,11 +52,13 @@ export const loadConfig: Dependencies['loadConfig'] = async function loadConfig( const configPath = asFsPath(configUri); const rootPath = asFsPath(rootUri); if (configExists) { - return nodeLoadConfig(configPath, rootPath).then(normalizeRoot); + return nodeLoadConfig(configPath, rootPath, options).then(normalizeRoot); } else if (isDefinitelyThemeAppExtension) { - return nodeLoadConfig('theme-check:theme-app-extension', rootPath).then(normalizeRoot); + return nodeLoadConfig('theme-check:theme-app-extension', rootPath, options).then( + normalizeRoot, + ); } else { - return nodeLoadConfig(undefined, rootPath).then(normalizeRoot); + return nodeLoadConfig(undefined, rootPath, options).then(normalizeRoot); } } else { // We can't load configs properly in remote environments. @@ -64,7 +71,7 @@ export const loadConfig: Dependencies['loadConfig'] = async function loadConfig( rootUri: path.normalize(rootUri), }; } -}; +} function normalizeRoot(config: Config) { config.rootUri = path.normalize(config.rootUri); diff --git a/packages/theme-language-server-node/src/index.ts b/packages/theme-language-server-node/src/index.ts index c74e1c65e..278fa1a00 100644 --- a/packages/theme-language-server-node/src/index.ts +++ b/packages/theme-language-server-node/src/index.ts @@ -1,5 +1,9 @@ import { ThemeLiquidDocsManager } from '@shopify/theme-check-docs-updater'; -import { AbstractFileSystem, NodeFileSystem } from '@shopify/theme-check-node'; +import { + AbstractFileSystem, + CustomCheckAuthorizationRequest, + NodeFileSystem, +} from '@shopify/theme-check-node'; import { startServer as startCoreServer } from '@shopify/theme-language-server-common'; import { stdin, stdout } from 'node:process'; import { createConnection } from 'vscode-languageserver/node'; @@ -11,7 +15,15 @@ export * from '@shopify/theme-language-server-common'; export const getConnection = () => createConnection(stdin, stdout); -export function startServer(connection = getConnection(), fs: AbstractFileSystem = NodeFileSystem) { +export interface StartServerOptions { + authorizeCustomChecks?: (request: CustomCheckAuthorizationRequest) => Promise; +} + +export function startServer( + connection = getConnection(), + fs: AbstractFileSystem = NodeFileSystem, + options: StartServerOptions = {}, +) { // Using console.error to not interfere with messages sent on STDIN/OUT const log = (message: string) => console.error(message); const themeLiquidDocsManager = new ThemeLiquidDocsManager(log); @@ -19,7 +31,8 @@ export function startServer(connection = getConnection(), fs: AbstractFileSystem startCoreServer(connection, { fs, log, - loadConfig, + loadConfig: (uri, fs) => + loadConfig(uri, fs, { authorizeCustomChecks: options.authorizeCustomChecks }), themeDocset: themeLiquidDocsManager, jsonValidationSet: themeLiquidDocsManager, fetchMetafieldDefinitionsForURI, diff --git a/packages/vscode-extension/src/node/customCheckConsent.spec.ts b/packages/vscode-extension/src/node/customCheckConsent.spec.ts new file mode 100644 index 000000000..ed1682404 --- /dev/null +++ b/packages/vscode-extension/src/node/customCheckConsent.spec.ts @@ -0,0 +1,102 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { window, workspace } from 'vscode'; +import { makeCustomCheckPermissionHandler } from './customCheckConsent'; + +vi.mock('vscode', () => ({ + window: { + showWarningMessage: vi.fn(), + }, + workspace: { + isTrusted: true, + }, +})); + +describe('custom check consent', () => { + const params = { + root: '/themes/example', + candidates: [{ source: 'require' as const, path: '/themes/example/checks/custom.js' }], + }; + + beforeEach(() => { + vi.clearAllMocks(); + Object.defineProperty(workspace, 'isTrusted', { configurable: true, value: true }); + }); + + it('denies custom checks without prompting in an untrusted workspace', async () => { + Object.defineProperty(workspace, 'isTrusted', { configurable: true, value: false }); + const { context } = makeContext(); + + const allowed = await makeCustomCheckPermissionHandler(context)(params); + + expect(allowed).toBe(false); + expect(window.showWarningMessage).not.toHaveBeenCalled(); + }); + + it('allows custom checks once after explicit consent', async () => { + vi.mocked(window.showWarningMessage).mockResolvedValue('Run once' as any); + const { context } = makeContext(); + const handler = makeCustomCheckPermissionHandler(context); + + expect(await handler(params)).toBe(true); + expect(await handler(params)).toBe(true); + expect(window.showWarningMessage).toHaveBeenCalledTimes(1); + }); + + it('keeps custom checks disabled when consent is declined', async () => { + vi.mocked(window.showWarningMessage).mockResolvedValue('Keep disabled' as any); + const { context } = makeContext(); + const handler = makeCustomCheckPermissionHandler(context); + + expect(await handler(params)).toBe(false); + expect(await handler(params)).toBe(false); + expect(window.showWarningMessage).toHaveBeenCalledTimes(1); + }); + + it('persists workspace consent', async () => { + vi.mocked(window.showWarningMessage).mockResolvedValue( + 'Always allow for this workspace' as any, + ); + const { context, update } = makeContext(); + + expect(await makeCustomCheckPermissionHandler(context)(params)).toBe(true); + vi.mocked(window.showWarningMessage).mockClear(); + expect(await makeCustomCheckPermissionHandler(context)(params)).toBe(true); + + expect(update).toHaveBeenCalledWith( + 'themeCheck.trustedCustomChecks', + expect.arrayContaining([expect.any(String)]), + ); + expect(window.showWarningMessage).not.toHaveBeenCalled(); + }); + + it('prompts again when the requested custom checks change', async () => { + vi.mocked(window.showWarningMessage).mockResolvedValue('Run once' as any); + const { context } = makeContext(); + const handler = makeCustomCheckPermissionHandler(context); + + await handler(params); + await handler({ + ...params, + candidates: [ + ...params.candidates, + { source: 'require', path: '/themes/example/checks/new.js' }, + ], + }); + + expect(window.showWarningMessage).toHaveBeenCalledTimes(2); + }); +}); + +function makeContext() { + let state: string[] = []; + const update = vi.fn(async (_key: string, value: string[]) => { + state = value; + }); + const context = { + workspaceState: { + get: vi.fn((_key: string, defaultValue: string[]) => state ?? defaultValue), + update, + }, + } as any; + return { context, update }; +} diff --git a/packages/vscode-extension/src/node/customCheckConsent.ts b/packages/vscode-extension/src/node/customCheckConsent.ts new file mode 100644 index 000000000..6680049d2 --- /dev/null +++ b/packages/vscode-extension/src/node/customCheckConsent.ts @@ -0,0 +1,100 @@ +import { createHash } from 'node:crypto'; +import path from 'node:path'; +import { CustomCheckPermissionRequest } from '@shopify/theme-language-server-common'; +import { ExtensionContext, window, workspace } from 'vscode'; + +const RUN_ONCE = 'Run once'; +const ALWAYS_ALLOW = 'Always allow for this workspace'; +const KEEP_DISABLED = 'Keep disabled'; +const TRUSTED_CUSTOM_CHECKS_KEY = 'themeCheck.trustedCustomChecks'; + +export function makeCustomCheckPermissionHandler(context: ExtensionContext) { + const sessionDecisions = new Map(); + const pendingDecisions = new Map>(); + + return async function requestCustomCheckPermission( + params: CustomCheckPermissionRequest.Params, + ): Promise { + if (!workspace.isTrusted) { + return false; + } + + const fingerprint = fingerprintRequest(params); + const trusted = context.workspaceState.get(TRUSTED_CUSTOM_CHECKS_KEY, []); + if (trusted.includes(fingerprint)) { + return true; + } + + const sessionDecision = sessionDecisions.get(fingerprint); + if (sessionDecision !== undefined) { + return sessionDecision; + } + + const pendingDecision = pendingDecisions.get(fingerprint); + if (pendingDecision) { + return pendingDecision; + } + + const decision = promptForPermission(context, params, fingerprint).finally(() => { + pendingDecisions.delete(fingerprint); + }); + pendingDecisions.set(fingerprint, decision); + const allowed = await decision; + sessionDecisions.set(fingerprint, allowed); + return allowed; + }; +} + +async function promptForPermission( + context: ExtensionContext, + params: CustomCheckPermissionRequest.Params, + fingerprint: string, +): Promise { + const choice = await window.showWarningMessage( + 'This theme wants to run custom Theme Check code.', + { + modal: true, + detail: permissionDetail(params), + }, + RUN_ONCE, + ALWAYS_ALLOW, + KEEP_DISABLED, + ); + + if (choice === ALWAYS_ALLOW) { + const trusted = context.workspaceState.get(TRUSTED_CUSTOM_CHECKS_KEY, []); + await context.workspaceState.update( + TRUSTED_CUSTOM_CHECKS_KEY, + Array.from(new Set([...trusted, fingerprint])), + ); + return true; + } + + return choice === RUN_ONCE; +} + +function permissionDetail(params: CustomCheckPermissionRequest.Params): string { + const candidates = params.candidates.map((candidate) => { + const relativePath = path.relative(params.root, candidate.path); + const displayPath = relativePath.startsWith('..') + ? candidate.path + : `.${path.sep}${relativePath}`; + return `• ${displayPath}`; + }); + + return [ + 'Custom checks execute JavaScript with your user permissions:', + '', + ...candidates, + '', + 'Only allow this if you trust the theme and these checks.', + ].join('\n'); +} + +function fingerprintRequest(params: CustomCheckPermissionRequest.Params): string { + const candidates = params.candidates + .map(({ source, path }) => `${source}:${path}`) + .sort() + .join('\n'); + return createHash('sha256').update(`${params.root}\n${candidates}`).digest('hex'); +} diff --git a/packages/vscode-extension/src/node/extension.ts b/packages/vscode-extension/src/node/extension.ts index 0bf9b99fc..74c3030a0 100644 --- a/packages/vscode-extension/src/node/extension.ts +++ b/packages/vscode-extension/src/node/extension.ts @@ -17,6 +17,8 @@ import { watchReferencesTreeViewConfig, } from '../common/ReferencesProvider'; import { makeDeadCode, openLocation } from '../common/commands'; +import { CustomCheckPermissionRequest } from '@shopify/theme-language-server-common'; +import { makeCustomCheckPermissionHandler } from './customCheckConsent'; const sleep = (ms: number) => new Promise((res) => setTimeout(res, ms)); @@ -89,6 +91,8 @@ async function startServer(context: ExtensionContext) { return workspace.fs.stat(Uri.parse(uriString)); }); + client.onRequest(CustomCheckPermissionRequest.type, makeCustomCheckPermissionHandler(context)); + client.start(); } diff --git a/packages/vscode-extension/src/node/server.ts b/packages/vscode-extension/src/node/server.ts index 15c239a8c..cf6f302b2 100644 --- a/packages/vscode-extension/src/node/server.ts +++ b/packages/vscode-extension/src/node/server.ts @@ -1,5 +1,6 @@ import type { AbstractFileSystem } from '@shopify/theme-check-common'; import { getConnection, NodeFileSystem, startServer } from '@shopify/theme-language-server-node'; +import { CustomCheckPermissionRequest } from '@shopify/theme-language-server-common'; import { VsCodeFileSystem } from '../common/VsCodeFileSystem'; const connection = getConnection(); @@ -9,7 +10,16 @@ const fileSystems: Record = { file: NodeFileSystem, }; -startServer(connection, new VsCodeFileSystem(connection, fileSystems)); +startServer(connection, new VsCodeFileSystem(connection, fileSystems), { + authorizeCustomChecks: async ({ root, candidates }) => { + try { + return await connection.sendRequest(CustomCheckPermissionRequest.type, { root, candidates }); + } catch { + // The VS Code client should fail closed if it cannot make a trust decision. + return false; + } + }, +}); process.on('uncaughtException', (e) => { console.error(e);