Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Increase security by virtualizing $env/static/* #5825

Merged
merged 18 commits into from Aug 16, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lovely-boxes-give.md
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

[feat] `$env/static/*` are now virtual to prevent writing sensitive values to disk
28 changes: 2 additions & 26 deletions packages/kit/src/core/sync/write_ambient.js
@@ -1,5 +1,4 @@
import path from 'path';
import colors from 'kleur';
import { get_env } from '../../vite/utils.js';
import { write_if_changed, reserved, valid_identifier } from './utils.js';

Expand All @@ -16,18 +15,6 @@ const types_reference = '/// <reference types="@sveltejs/kit" />\n\n';
export function write_ambient(config, mode) {
const env = get_env(mode, config.env.publicPrefix);

// TODO when testing src, `$app` points at `src/runtime/app`... will
// probably need to fiddle with aliases
write_if_changed(
path.join(config.outDir, 'runtime/env/static/public.js'),
create_env_module('$env/static/public', env.public)
);

write_if_changed(
path.join(config.outDir, 'runtime/env/static/private.js'),
create_env_module('$env/static/private', env.private)
);

write_if_changed(
path.join(config.outDir, 'ambient.d.ts'),
autogen_comment +
Expand All @@ -43,23 +30,12 @@ export function write_ambient(config, mode) {
* @param {Record<string, string>} env
* @returns {string}
*/
function create_env_module(id, env) {
export function create_env_module(id, env) {
/** @type {string[]} */
const declarations = [];

for (const key in env) {
const warning = !valid_identifier.test(key)
? 'not a valid identifier'
: reserved.has(key)
? 'a reserved word'
: null;

if (warning) {
console.error(
colors
.bold()
.yellow(`Omitting environment variable "${key}" from ${id} as it is ${warning}`)
);
if (!valid_identifier.test(key) || reserved.has(key)) {
continue;
}

Expand Down
55 changes: 51 additions & 4 deletions packages/kit/src/vite/index.js
Expand Up @@ -14,8 +14,9 @@ import { generate_manifest } from '../core/generate_manifest/index.js';
import { get_runtime_directory, logger } from '../core/utils.js';
import { find_deps, get_default_config as get_default_build_config } from './build/utils.js';
import { preview } from './preview/index.js';
import { get_aliases, resolve_entry, prevent_illegal_rollup_imports } from './utils.js';
import { get_aliases, resolve_entry, prevent_illegal_rollup_imports, get_env } from './utils.js';
import { fileURLToPath } from 'node:url';
import { create_env_module } from '../core/sync/write_ambient.js';

const cwd = process.cwd();

Expand Down Expand Up @@ -61,7 +62,53 @@ const enforced_config = {
* @return {import('vite').Plugin[]}
*/
export function sveltekit() {
return [...svelte(), kit()];
return [...svelte(), env(), kit()];
}

/**
* Returns the SvelteKit environment plugin. Replaces `$env/static/*` imports
* with the actual environment variables.
* @return {import('vite').Plugin}
*/
function env() {
/** @type {import('types').ValidatedConfig} */
let svelte_config;

/** @type {import('vite').ConfigEnv} */
let vite_config_env;
tcc-sejohnson marked this conversation as resolved.
Show resolved Hide resolved

return {
name: 'vite-plugin-svelte-kit:env',

async config(_, config_env) {
vite_config_env = config_env;
svelte_config = await load_config();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there were previous efforts to avoid reading svelte config multiple times, as it may lead to unexpected side effects when it contains TLA initialization code.

To avoid this here we'd either have to steal the ref from kit plugin somehow or move the resolve + load hooks there

cc @benmccann

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we could load multiple times as long as we use the same loading helper (just have to make sure the loading helper uses the mtime trick)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let you guys duke this one out as I have no idea one way or the other. I like this as its own plugin (it's just way less confusing and way more obvious what it's doing), but I can move it back into the main plugin if there's a need.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it into the main plugin — if we do get round to creating a helper that allows us to load the config multiple times without penalty then we should, but in the meantime i think it's better that we load it as few times as possible

},

async resolveId(id) {
switch (id) {
case '$env/static/private':
return '\0$env/static/private';
case '$env/static/public':
return '\0$env/static/public';
}
},

async load(id) {
switch (id) {
case '\0$env/static/private':
return create_env_module(
'$env/static/private',
get_env(vite_config_env.mode, svelte_config.kit.env.publicPrefix).private
);
case '\0$env/static/public':
return create_env_module(
'$env/static/private',
tcc-sejohnson marked this conversation as resolved.
Show resolved Hide resolved
get_env(vite_config_env.mode, svelte_config.kit.env.publicPrefix).public
);
}
}
};
}

/**
Expand Down Expand Up @@ -411,8 +458,8 @@ function kit() {
console.log(colors.bold().yellow('\nNo adapter specified'));
// prettier-ignore
console.log(
`See ${colors.bold().cyan('https://kit.svelte.dev/docs/adapters')} to learn how to configure your app to run on the platform of your choosing`
);
`See ${colors.bold().cyan('https://kit.svelte.dev/docs/adapters')} to learn how to configure your app to run on the platform of your choosing`
);
}
},

Expand Down
16 changes: 7 additions & 9 deletions packages/kit/src/vite/utils.js
Expand Up @@ -128,24 +128,22 @@ export function get_aliases(config) {
}
}

const base = process.env.BUNDLED
? `${get_runtime_directory(config)}/env`
: path.posix.join(config.outDir, 'runtime/env');
if (!process.env.BUNDLED) {
alias.push(
{
find: '$env/static/public',
replacement: path.posix.join(config.outDir, 'runtime/env/static/public.js')
find: '$env/dynamic/public',
replacement: `${base}/dynamic/public.js`
},
{
find: '$env/static/private',
replacement: path.posix.join(config.outDir, 'runtime/env/static/private.js')
find: '$env/dynamic/private',
replacement: `${base}/dynamic/private.js`
}
);
}

alias.push({
find: '$env',
replacement: `${get_runtime_directory(config)}/env`
});

return alias;
}

Expand Down
12 changes: 4 additions & 8 deletions packages/kit/src/vite/utils.spec.js
Expand Up @@ -236,16 +236,12 @@ test('transform kit.alias to resolve.alias', () => {
{ find: /^key$/.toString(), replacement: 'value' },
{ find: /^key\/(.+)$/.toString(), replacement: 'value/$1' },
{
find: '$env/static/public',
replacement: '.svelte-kit/runtime/env/static/public.js'
find: '$env/dynamic/public',
replacement: '.svelte-kit/runtime/env/dynamic/public.js'
},
{
find: '$env/static/private',
replacement: '.svelte-kit/runtime/env/static/private.js'
},
{
find: '$env',
replacement: 'src/runtime/env'
find: '$env/dynamic/private',
replacement: '.svelte-kit/runtime/env/dynamic/private.js'
}
]);
});
Expand Down