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.

Sign up for GitHub

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

Jump to bottom

Prefer (allowed) static assets to routes #5070

Merged
merged 13 commits into from
Jul 8, 2022
Merged

Prefer (allowed) static assets to routes #5070

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
480d38f
failing test for #5066
Rich-Harris May 25, 2022
0d9f8cf
Merge branch 'master' into gh-5066
Rich-Harris Jul 5, 2022
d36f145
allow vite to serve files where appropriate
Rich-Harris Jul 6, 2022
f0591b1
print helpful 403 in dev
Rich-Harris Jul 6, 2022
adb5a47
Update packages/kit/src/vite/dev/index.js
Rich-Harris Jul 6, 2022
6fc2ea0
Merge branch 'master' into gh-5066
Rich-Harris Jul 6, 2022
8a720df
try and figure out why windows is failing
Rich-Harris Jul 6, 2022
d03f554
try this
Rich-Harris Jul 6, 2022
52f74a4
store protocol and verbose instead of vite_config
Rich-Harris Jul 6, 2022
0428032
neaten up
Rich-Harris Jul 6, 2022
dbe6946
account for server.fs.strict
Rich-Harris Jul 6, 2022
04b49e8
Update packages/kit/test/apps/basics/test/test.js
Rich-Harris Jul 7, 2022
1bce523
Create nice-boxes-exercise.md
Rich-Harris Jul 8, 2022
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
15 changes: 14 additions & 1 deletion packages/kit/src/vite/dev/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ const cwd = process.cwd();

/**
* @param {import('vite').ViteDevServer} vite
* @param {import('vite').ResolvedConfig} vite_config
* @param {import('types').ValidatedConfig} svelte_config
* @return {Promise<Promise<() => void>>}
*/
export async function dev(vite, svelte_config) {
export async function dev(vite, vite_config, svelte_config) {
installPolyfills();

sync.init(svelte_config);
Expand Down Expand Up @@ -212,6 +213,18 @@ export async function dev(vite, svelte_config) {
}
}

const file = posixify(path.resolve(decoded.slice(1)));
const is_file = fs.existsSync(file) && !fs.statSync(file).isDirectory();
const allowed =
!vite_config.server.fs.strict ||
vite_config.server.fs.allow.some((dir) => file.startsWith(dir));

if (is_file && allowed) {
// @ts-expect-error
serve_static_middleware.handle(req, res);
return;
}

if (!decoded.startsWith(svelte_config.kit.paths.base)) {
return not_found(
res,
Expand Down
17 changes: 10 additions & 7 deletions packages/kit/src/vite/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function kit() {
/** @type {import('types').ValidatedConfig} */
let svelte_config;

/** @type {import('vite').UserConfig} */
/** @type {import('vite').ResolvedConfig} */
let vite_config;

/** @type {import('vite').ConfigEnv} */
Expand Down Expand Up @@ -121,7 +121,6 @@ function kit() {
name: 'vite-plugin-svelte-kit',

async config(config, config_env) {
vite_config = config;
vite_config_env = config_env;
svelte_config = await load_config();
is_build = config_env.command === 'build';
Expand Down Expand Up @@ -191,6 +190,10 @@ function kit() {
return result;
},

configResolved(config) {
vite_config = config;
},

buildStart() {
if (is_build) {
rimraf(paths.build_dir);
Expand All @@ -202,8 +205,9 @@ function kit() {
},

async writeBundle(_options, bundle) {
const verbose = vite_config.logLevel === 'info';
const log = logger({ verbose });
const log = logger({
verbose: vite_config.logLevel === 'info'
});

fs.writeFileSync(
`${paths.client_out_dir}/version.json`,
Expand Down Expand Up @@ -336,12 +340,11 @@ function kit() {
},

async configureServer(vite) {
return await dev(vite, svelte_config);
return await dev(vite, vite_config, svelte_config);
},

configurePreviewServer(vite) {
const protocol = vite_config.preview?.https ? 'https' : 'http';
return preview(vite, svelte_config, protocol);
return preview(vite, svelte_config, vite_config.preview.https ? 'https' : 'http');
}
};
}
Expand Down
7 changes: 7 additions & 0 deletions packages/kit/test/apps/basics/src/routes/package.json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export function get() {
return {
body: {
works: true
}
};
}
6 changes: 6 additions & 0 deletions packages/kit/test/apps/basics/src/routes/src/[...anything].js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/** @type {import('./__types/[...anything]').RequestHandler} */
export function get() {
return {
body: 'dynamically rendered file'
};
}
11 changes: 8 additions & 3 deletions packages/kit/test/apps/basics/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2572,12 +2572,17 @@ test.describe('Static files', () => {
expect(response.status()).toBe(process.env.DEV ? 403 : 404);
});

test('Vite serves assets in src directory', async ({ page, request }) => {
test('Vite serves assets in allowed directories', async ({ page, request }) => {
await page.goto('/assets');
const path = await page.textContent('h1');

const response = await request.get(path);
expect(response.status()).toBe(200);
const r1 = await request.get(path);
expect(r1.status()).toBe(200);
expect(await r1.text()).toContain('http://www.w3.org/2000/svg');

const r2 = await request.get('/package.json');
Copy link
Member

Choose a reason for hiding this comment

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

this seems surprising to me. I wouldn't expect you to be able to get the package.json, node_modules, etc. It will also make it available to everyone on your network. I'm not sure that's a huge risk, but I'm not really sure why we'd expose it either

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not getting package.json. The test is there to check that it will serve a route called /package.json despite there being a package.json in the project directory outside the allow list. Check two lines further down.

node_modules on the other hand are on the allow list so that you can use node_modules!

Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
expect(r2.status()).toBe(200);
expect(await r2.json()).toEqual({ works: true });
});

test('Filenames are case-sensitive', async ({ request }) => {
Expand Down