feat: Disallow import of user-designated server-only modules from the…

… client (#6422) * feat: Failing tests * feat: Dev-only tests * feat: It works...? * changeset * fix: Don't re-resolve $lib * fix: Dumb test * fix: More dumb tests * fix: Even more dumb tests * fix: wut * fix: vscode autoimport is a miserable bully * fix: Workspace? * fix: format * fix: jsconfig => tsconfig * fix: Some svelte-check misery * fix: noEmit * fix: Windows paths are miserable funsuckers * feat: Better documentation of server-only stuff * Revert "feat: Better documentation of server-only stuff" This reverts commit 6b584b8. * feat: Switch to "not in node_modules" logic * broken lockfile, apparently * fix * Update packages/kit/src/exports/vite/utils.js Co-authored-by: Rich Harris <richard.a.harris@gmail.com> * simplify * simplify * lockfile * DRY out, ignore files outside project root * bruh * bruh 2.0 * bruh 3.0 * Update packages/kit/src/exports/vite/utils.js Co-authored-by: Rich Harris <hello@rich-harris.dev> Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
sveltejs · Sep 5, 2022 · 0032048 · 0032048
1 parent 55e9151
commit 0032048
Show file tree

Hide file tree

Showing 43 changed files with 456 additions and 72 deletions.
diff --git a/.changeset/perfect-rules-burn.md b/.changeset/perfect-rules-burn.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[breaking] Allow users to designate modules as server-only
diff --git a/packages/kit/src/exports/vite/dev/index.js b/packages/kit/src/exports/vite/dev/index.js
@@ -13,6 +13,7 @@ import * as sync from '../../../core/sync/sync.js';
 import { get_mime_lookup, runtime_base, runtime_prefix } from '../../../core/utils.js';
 import { prevent_illegal_vite_imports, resolve_entry } from '../utils.js';
 import { compact } from '../../../utils/array.js';
+import { normalizePath } from 'vite';
 
 // Vite doesn't expose this so we just copy the list for now
 // https://github.com/vitejs/vite/blob/3edd1af56e980aef56641a5a51cf2932bb580d41/packages/vite/src/node/plugins/css.ts#L96
@@ -24,10 +25,9 @@ const cwd = process.cwd();
  * @param {import('vite').ViteDevServer} vite
  * @param {import('vite').ResolvedConfig} vite_config
  * @param {import('types').ValidatedConfig} svelte_config
- * @param {Set<string>} illegal_imports
  * @return {Promise<Promise<() => void>>}
  */
-export async function dev(vite, vite_config, svelte_config, illegal_imports) {
+export async function dev(vite, vite_config, svelte_config) {
 	installPolyfills();
 
 	sync.init(svelte_config, vite_config.mode);
@@ -90,7 +90,11 @@ export async function dev(vite, vite_config, svelte_config, illegal_imports) {
 								module_nodes.push(module_node);
 								result.file = url.endsWith('.svelte') ? url : url + '?import'; // TODO what is this for?
 
-								prevent_illegal_vite_imports(module_node, illegal_imports, extensions);
+								prevent_illegal_vite_imports(
+									module_node,
+									normalizePath(svelte_config.kit.files.lib),
+									extensions
+								);
 
 								return module.default;
 							};
@@ -103,7 +107,11 @@ export async function dev(vite, vite_config, svelte_config, illegal_imports) {
 
 							result.shared = module;
 
-							prevent_illegal_vite_imports(module_node, illegal_imports, extensions);
+							prevent_illegal_vite_imports(
+								module_node,
+								normalizePath(svelte_config.kit.files.lib),
+								extensions
+							);
 						}
 
 						if (node.server) {

diff --git a/packages/kit/src/exports/vite/index.js b/packages/kit/src/exports/vite/index.js
@@ -103,9 +103,6 @@ function kit() {
 	/** @type {import('types').BuildData} */
 	let build_data;
 
-	/** @type {Set<string>} */
-	let illegal_imports;
-
 	/** @type {string | undefined} */
 	let deferred_warning;
 
@@ -212,13 +209,6 @@ function kit() {
 				client_out_dir: `${svelte_config.kit.outDir}/output/client`
 			};
 
-			illegal_imports = new Set([
-				'/@id/__x00__$env/dynamic/private', //dev
-				'\0$env/dynamic/private', // prod
-				'/@id/__x00__$env/static/private', // dev
-				'\0$env/static/private' // prod
-			]);
-
 			if (is_build) {
 				manifest_data = (await sync.all(svelte_config, config_env.mode)).manifest_data;
 
@@ -361,7 +351,7 @@ function kit() {
 						prevent_illegal_rollup_imports(
 							this.getModuleInfo.bind(this),
 							module_node,
-							illegal_imports
+							vite.normalizePath(svelte_config.kit.files.lib)
 						);
 					}
 				});
@@ -517,7 +507,7 @@ function kit() {
 				if (deferred_warning) console.error('\n' + deferred_warning);
 			};
 
-			return await dev(vite, vite_config, svelte_config, illegal_imports);
+			return await dev(vite, vite_config, svelte_config);
 		},
 
 		/**

diff --git a/packages/kit/src/exports/vite/utils.js b/packages/kit/src/exports/vite/utils.js
@@ -4,6 +4,26 @@ import { loadConfigFromFile, loadEnv, normalizePath } from 'vite';
 import { runtime_directory } from '../../core/utils.js';
 import { posixify } from '../../utils/filesystem.js';
 
+const illegal_imports = new Set([
+	'/@id/__x00__$env/dynamic/private', //dev
+	'\0$env/dynamic/private', // prod
+	'/@id/__x00__$env/static/private', // dev
+	'\0$env/static/private' // prod
+]);
+
+/** @param {string} id */
+function is_illegal(id) {
+	if (illegal_imports.has(id)) return true;
+
+	// files outside the project root are ignored
+	if (!id.startsWith(normalizePath(process.cwd()))) return false;
+
+	// so are files inside node_modules
+	if (id.startsWith(normalizePath(node_modules_dir))) return false;
+
+	return /.*\.server\..+/.test(path.basename(id));
+}
+
 /**
  * @param {import('vite').ResolvedConfig} config
  * @param {import('vite').ConfigEnv} config_env
@@ -183,8 +203,9 @@ function repeat(str, times) {
 /**
  * Create a formatted error for an illegal import.
  * @param {Array<{name: string, dynamic: boolean}>} stack
+ * @param {string} lib_dir
  */
-function format_illegal_import_chain(stack) {
+function format_illegal_import_chain(stack, lib_dir) {
 	const dev_virtual_prefix = '/@id/__x00__';
 	const prod_virtual_prefix = '\0';
 
@@ -195,6 +216,9 @@ function format_illegal_import_chain(stack) {
 		if (file.name.startsWith(prod_virtual_prefix)) {
 			return { ...file, name: file.name.replace(prod_virtual_prefix, '') };
 		}
+		if (file.name.startsWith(lib_dir)) {
+			return { ...file, name: file.name.replace(lib_dir, '$lib') };
+		}
 
 		return { ...file, name: path.relative(process.cwd(), file.name) };
 	});
@@ -211,6 +235,8 @@ function format_illegal_import_chain(stack) {
 	return `Cannot import ${stack.at(-1)?.name} into client-side code:\n${pyramid}`;
 }
 
+const node_modules_dir = path.resolve(process.cwd(), 'node_modules');
+
 /**
  * Load environment variables from process.env and .env files
  * @param {import('types').ValidatedKitConfig['env']} env_config
@@ -228,11 +254,11 @@ export function get_env(env_config, mode) {
 /**
  * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter
  * @param {import('rollup').ModuleInfo} node
- * @param {Set<string>} illegal_imports Illegal module IDs -- be sure to call vite.normalizePath!
+ * @param {string} lib_dir
  */
-export function prevent_illegal_rollup_imports(node_getter, node, illegal_imports) {
-	const chain = find_illegal_rollup_imports(node_getter, node, false, illegal_imports);
-	if (chain) throw new Error(format_illegal_import_chain(chain));
+export function prevent_illegal_rollup_imports(node_getter, node, lib_dir) {
+	const chain = find_illegal_rollup_imports(node_getter, node, false);
+	if (chain) throw new Error(format_illegal_import_chain(chain, lib_dir));
 }
 
 const query_pattern = /\?.*$/s;
@@ -246,36 +272,27 @@ function remove_query_from_path(path) {
  * @param {(id: string) => import('rollup').ModuleInfo | null} node_getter
  * @param {import('rollup').ModuleInfo} node
  * @param {boolean} dynamic
- * @param {Set<string>} illegal_imports Illegal module IDs -- be sure to call vite.normalizePath!
  * @param {Set<string>} seen
  * @returns {Array<import('types').ImportNode> | null}
  */
-const find_illegal_rollup_imports = (
-	node_getter,
-	node,
-	dynamic,
-	illegal_imports,
-	seen = new Set()
-) => {
+const find_illegal_rollup_imports = (node_getter, node, dynamic, seen = new Set()) => {
 	const name = remove_query_from_path(normalizePath(node.id));
 	if (seen.has(name)) return null;
 	seen.add(name);
 
-	if (illegal_imports.has(name)) {
+	if (is_illegal(name)) {
 		return [{ name, dynamic }];
 	}
 
 	for (const id of node.importedIds) {
 		const child = node_getter(id);
-		const chain =
-			child && find_illegal_rollup_imports(node_getter, child, false, illegal_imports, seen);
+		const chain = child && find_illegal_rollup_imports(node_getter, child, false, seen);
 		if (chain) return [{ name, dynamic }, ...chain];
 	}
 
 	for (const id of node.dynamicallyImportedIds) {
 		const child = node_getter(id);
-		const chain =
-			child && find_illegal_rollup_imports(node_getter, child, true, illegal_imports, seen);
+		const chain = child && find_illegal_rollup_imports(node_getter, child, true, seen);
 		if (chain) return [{ name, dynamic }, ...chain];
 	}
 
@@ -308,22 +325,21 @@ const get_module_types = (config_module_types) => {
 /**
  * Throw an error if a private module is imported from a client-side node.
  * @param {import('vite').ModuleNode} node
- * @param {Set<string>} illegal_imports Illegal module IDs -- be sure to call vite.normalizePath!
+ * @param {string} lib_dir
  * @param {Iterable<string>} module_types File extensions to analyze in addition to the defaults: `.ts`, `.js`, etc.
  */
-export function prevent_illegal_vite_imports(node, illegal_imports, module_types) {
-	const chain = find_illegal_vite_imports(node, illegal_imports, get_module_types(module_types));
-	if (chain) throw new Error(format_illegal_import_chain(chain));
+export function prevent_illegal_vite_imports(node, lib_dir, module_types) {
+	const chain = find_illegal_vite_imports(node, get_module_types(module_types));
+	if (chain) throw new Error(format_illegal_import_chain(chain, lib_dir));
 }
 
 /**
  * @param {import('vite').ModuleNode} node
- * @param {Set<string>} illegal_imports Illegal module IDs -- be sure to call vite.normalizePath!
  * @param {Set<string>} module_types File extensions to analyze: `.ts`, `.js`, etc.
  * @param {Set<string>} seen
  * @returns {Array<import('types').ImportNode> | null}
  */
-function find_illegal_vite_imports(node, illegal_imports, module_types, seen = new Set()) {
+function find_illegal_vite_imports(node, module_types, seen = new Set()) {
 	if (!node.id) return null; // TODO when does this happen?
 	const name = remove_query_from_path(normalizePath(node.id));
 
@@ -332,12 +348,12 @@ function find_illegal_vite_imports(node, illegal_imports, module_types, seen = n
 	}
 	seen.add(name);
 
-	if (name && illegal_imports.has(name)) {
+	if (is_illegal(name)) {
 		return [{ name, dynamic: false }];
 	}
 
 	for (const child of node.importedModules) {
-		const chain = child && find_illegal_vite_imports(child, illegal_imports, module_types, seen);
+		const chain = child && find_illegal_vite_imports(child, module_types, seen);
 		if (chain) return [{ name, dynamic: false }, ...chain];
 	}
 

diff --git a/packages/kit/src/exports/vite/utils.spec.js b/packages/kit/src/exports/vite/utils.spec.js
@@ -1,7 +1,6 @@
 import path from 'path';
 import { test } from 'uvu';
 import * as assert from 'uvu/assert';
-import { normalizePath } from 'vite';
 import { validate_config } from '../../core/config/index.js';
 import { posixify } from '../../utils/filesystem.js';
 import {
@@ -12,6 +11,8 @@ import {
 	prevent_illegal_vite_imports
 } from './utils.js';
 
+const illegal_id = '/@id/__x00__$env/dynamic/private';
+
 test('basic test no conflicts', async () => {
 	const merged = deep_merge(
 		{
@@ -277,7 +278,7 @@ const rollup_node_getter = (id) => {
 		},
 		'/statically-imports/bad/module.js': {
 			id: '/statically-imports/bad/module.js',
-			importedIds: ['/illegal/boom.js'],
+			importedIds: [illegal_id],
 			dynamicallyImportedIds: ['/test/path2.js']
 		},
 		'/bad/dynamic.js': {
@@ -288,43 +289,50 @@ const rollup_node_getter = (id) => {
 		'/dynamically-imports/bad/module.js': {
 			id: '/dynamically-imports/bad/module.js',
 			importedIds: ['/test/path5.js'],
-			dynamicallyImportedIds: ['/test/path2.js', '/illegal/boom.js']
+			dynamicallyImportedIds: ['/test/path2.js', illegal_id]
 		},
-		'/illegal/boom.js': {
-			id: '/illegal/boom.js',
+		[illegal_id]: {
+			id: illegal_id,
 			importedIds: [],
 			dynamicallyImportedIds: []
 		}
 	};
 	return nodes[id] ?? null;
 };
 
-const illegal_imports = new Set([normalizePath('/illegal/boom.js')]);
 const ok_rollup_node = rollup_node_getter('/test/path1.js');
 const bad_rollup_node_static = rollup_node_getter('/bad/static.js');
 const bad_rollup_node_dynamic = rollup_node_getter('/bad/dynamic.js');
 
 test('allows ok rollup imports', () => {
 	assert.not.throws(() => {
-		// @ts-ignore
-		prevent_illegal_rollup_imports(rollup_node_getter, ok_rollup_node, illegal_imports, '');
+		prevent_illegal_rollup_imports(
+			// @ts-expect-error
+			rollup_node_getter,
+			ok_rollup_node,
+			'should_not_match_anything'
+		);
 	});
 });
 
 test('does not allow bad static rollup imports', () => {
 	assert.throws(() => {
-		// @ts-ignore
-		prevent_illegal_rollup_imports(rollup_node_getter, bad_rollup_node_static, illegal_imports, '');
+		prevent_illegal_rollup_imports(
+			// @ts-expect-error
+			rollup_node_getter,
+			bad_rollup_node_static,
+			'should_not_match_anything'
+		);
 	});
 });
 
 test('does not allow bad dynamic rollup imports', () => {
 	assert.throws(() => {
 		prevent_illegal_rollup_imports(
-			// @ts-ignore
+			// @ts-expect-error
 			rollup_node_getter,
 			bad_rollup_node_dynamic,
-			illegal_imports
+			'should_not_match_anything'
 		);
 	});
 });
@@ -359,7 +367,7 @@ const bad_vite_node = {
 					id: '/test/path2.js',
 					importedModules: new Set([
 						{
-							id: '/illegal/boom.js',
+							id: illegal_id,
 							importedModules: new Set()
 						}
 					])
@@ -372,15 +380,23 @@ const bad_vite_node = {
 
 test('allows ok vite imports', () => {
 	assert.not.throws(() => {
-		// @ts-ignore
-		prevent_illegal_vite_imports(ok_vite_node, illegal_imports, '');
+		prevent_illegal_vite_imports(
+			// @ts-expect-error
+			ok_vite_node,
+			'should_not_match_anything',
+			[]
+		);
 	});
 });
 
 test('does not allow bad static rollup imports', () => {
 	assert.throws(() => {
-		// @ts-ignore
-		prevent_illegal_vite_imports(bad_vite_node, illegal_imports, '');
+		prevent_illegal_vite_imports(
+			// @ts-expect-error
+			bad_vite_node,
+			'should_not_match_anything',
+			[]
+		);
 	});
 });
 

diff --git a/packages/kit/test/apps/dev-only/src/lib/test.server.js b/packages/kit/test/apps/dev-only/src/lib/test.server.js
@@ -0,0 +1 @@
+export const should_explode = 'boom';
diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/dynamic-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/server-only-modules/dynamic-import/+page.svelte
@@ -0,0 +1,7 @@
+<script>
+	const mod = import('$lib/test.server.js');
+</script>
+
+{#await mod then resolved}
+	<p>{resolved.should_explode}</p>
+{/await}
diff --git a/packages/kit/test/apps/dev-only/src/routes/server-only-modules/static-import/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/server-only-modules/static-import/+page.svelte
@@ -0,0 +1,5 @@
+<script>
+	import { should_explode } from '$lib/test.server.js';
+</script>
+
+<p>{should_explode}</p>