From eea9fe2744af234009953ccf6a3016f6e0f3a290 Mon Sep 17 00:00:00 2001 From: patak-js Date: Tue, 19 Oct 2021 21:55:26 +0200 Subject: [PATCH 1/7] fix: restrict static middleware fs access --- .../fs-serve/__tests__/fs-serve.spec.ts | 61 ++++++++++++------- packages/playground/fs-serve/root/.env | 1 + packages/playground/fs-serve/root/src/.env | 1 + .../fs-serve/root/{ => src}/index.html | 47 +++++++++++--- .../playground/fs-serve/root/vite.config.js | 9 ++- packages/vite/src/node/server/index.ts | 2 +- .../src/node/server/middlewares/static.ts | 18 ++++-- 7 files changed, 104 insertions(+), 35 deletions(-) create mode 100644 packages/playground/fs-serve/root/.env create mode 100644 packages/playground/fs-serve/root/src/.env rename packages/playground/fs-serve/root/{ => src}/index.html (53%) diff --git a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts index af09045be48b1a..49d0ffd0d57bf7 100644 --- a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts +++ b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts @@ -3,30 +3,47 @@ import { isBuild } from '../../testUtils' const json = require('../safe.json') const stringified = JSON.stringify(json) -if (!isBuild) { - test('default import', async () => { - expect(await page.textContent('.full')).toBe(stringified) +describe('main', () => { + beforeAll(async () => { + // viteTestUrl is globally injected in scripts/jestPerTestSetup.ts + await page.goto(viteTestUrl + '/src/') }) - test('named import', async () => { - expect(await page.textContent('.named')).toBe(json.msg) - }) + if (!isBuild) { + test('default import', async () => { + expect(await page.textContent('.full')).toBe(stringified) + }) - test('safe fetch', async () => { - expect(await page.textContent('.safe-fetch')).toBe(stringified) - expect(await page.textContent('.safe-fetch-status')).toBe('200') - }) + test('named import', async () => { + expect(await page.textContent('.named')).toBe(json.msg) + }) - test('unsafe fetch', async () => { - expect(await page.textContent('.unsafe-fetch')).toBe('') - expect(await page.textContent('.unsafe-fetch-status')).toBe('403') - }) + test('safe fetch', async () => { + expect(await page.textContent('.safe-fetch')).toMatch('KEY=safe') + expect(await page.textContent('.safe-fetch-status')).toBe('200') + }) - test('nested entry', async () => { - expect(await page.textContent('.nested-entry')).toBe('foobar') - }) -} else { - test('dummy test to make jest happy', async () => { - // Your test suite must contain at least one test. - }) -} + test('unsafe fetch', async () => { + expect(await page.textContent('.unsafe-fetch')).toBe('') + expect(await page.textContent('.unsafe-fetch-status')).toBe('404') // TODO: should be 403 + }) + + test('safe fs fetch', async () => { + expect(await page.textContent('.safe-fs-fetch')).toBe(stringified) + expect(await page.textContent('.safe-fs-fetch-status')).toBe('200') + }) + + test('unsafe fs fetch', async () => { + expect(await page.textContent('.unsafe-fs-fetch')).toBe('') + expect(await page.textContent('.unsafe-fs-fetch-status')).toBe('403') + }) + + test('nested entry', async () => { + expect(await page.textContent('.nested-entry')).toBe('foobar') + }) + } else { + test('dummy test to make jest happy', async () => { + // Your test suite must contain at least one test. + }) + } +}) diff --git a/packages/playground/fs-serve/root/.env b/packages/playground/fs-serve/root/.env new file mode 100644 index 00000000000000..d0e0cfd28cbe57 --- /dev/null +++ b/packages/playground/fs-serve/root/.env @@ -0,0 +1 @@ +KEY=unsafe diff --git a/packages/playground/fs-serve/root/src/.env b/packages/playground/fs-serve/root/src/.env new file mode 100644 index 00000000000000..3f3d0607101642 --- /dev/null +++ b/packages/playground/fs-serve/root/src/.env @@ -0,0 +1 @@ +KEY=safe diff --git a/packages/playground/fs-serve/root/index.html b/packages/playground/fs-serve/root/src/index.html similarity index 53% rename from packages/playground/fs-serve/root/index.html rename to packages/playground/fs-serve/root/src/index.html index 1f100557ba3e5b..67a2371c6b27fb 100644 --- a/packages/playground/fs-serve/root/index.html +++ b/packages/playground/fs-serve/root/src/index.html @@ -1,3 +1,5 @@ + +

Normal Import


 

@@ -10,34 +12,65 @@ 
Unsafe Fetch

 

 

 
+
Safe /@fs/ Fetch

+

+

+
+
Unsafe /@fs/ Fetch

+

+

+
 
Nested Entry

 

 
 
+
\ No newline at end of file
diff --git a/packages/playground/fs-serve/root/vite.config.js b/packages/playground/fs-serve/root/vite.config.js
index 1ae96c3d1f7350..585a91f9d6346d 100644
--- a/packages/playground/fs-serve/root/vite.config.js
+++ b/packages/playground/fs-serve/root/vite.config.js
@@ -4,10 +4,17 @@ const path = require('path')
  * @type {import('vite').UserConfig}
  */
 module.exports = {
+  build: {
+    rollupOptions: {
+      input: {
+        main: path.resolve(__dirname, 'src/index.html')
+      }
+    }
+  },
   server: {
     fs: {
       strict: true,
-      allow: [__dirname]
+      allow: [path.resolve(__dirname, 'src')]
     },
     hmr: {
       overlay: false
diff --git a/packages/vite/src/node/server/index.ts b/packages/vite/src/node/server/index.ts
index 8d692f5ec3e82a..42a1b1b5601bdb 100644
--- a/packages/vite/src/node/server/index.ts
+++ b/packages/vite/src/node/server/index.ts
@@ -525,7 +525,7 @@ export async function createServer(
 
   // serve static files
   middlewares.use(serveRawFsMiddleware(server))
-  middlewares.use(serveStaticMiddleware(root, config))
+  middlewares.use(serveStaticMiddleware(root, server))
 
   // spa fallback
   if (!middlewareMode || middlewareMode === 'html') {
diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index 8bff43a1f1ab69..0ad68870de2cb3 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -1,7 +1,7 @@
 import path from 'path'
 import sirv, { Options } from 'sirv'
 import { Connect } from 'types/connect'
-import { normalizePath, ResolvedConfig, ViteDevServer } from '../..'
+import { normalizePath, ViteDevServer } from '../..'
 import { FS_PREFIX } from '../../constants'
 import {
   cleanUrl,
@@ -45,12 +45,12 @@ export function servePublicMiddleware(dir: string): Connect.NextHandleFunction {
 
 export function serveStaticMiddleware(
   dir: string,
-  config: ResolvedConfig
+  server: ViteDevServer
 ): Connect.NextHandleFunction {
   const serve = sirv(dir, sirvOptions)
 
   // Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
-  return function viteServeStaticMiddleware(req, res, next) {
+  return async function viteServeStaticMiddleware(req, res, next) {
     // only serve the file if it's not an html request
     // so that html requests can fallthrough to our html middleware for
     // special processing
@@ -66,7 +66,7 @@ export function serveStaticMiddleware(
 
     // apply aliases to static requests as well
     let redirected: string | undefined
-    for (const { find, replacement } of config.resolve.alias) {
+    for (const { find, replacement } of server.config.resolve.alias) {
       const matches =
         typeof find === 'string' ? url.startsWith(find) : find.test(url)
       if (matches) {
@@ -79,6 +79,16 @@ export function serveStaticMiddleware(
       if (redirected.startsWith(dir)) {
         redirected = redirected.slice(dir.length)
       }
+    }
+
+    const resolvedUrl = redirected || url
+    const fileUrl = path.resolve(dir, resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl)
+    // TODO: should use ensureServingAccess(fileUrl, server)
+    if (!isFileServingAllowed(fileUrl, server)) {
+      return next()
+    }
+
+    if (redirected) {
       req.url = redirected
     }
 

From b9358e310051f05dab816c3cb0953eafd2e1cadf Mon Sep 17 00:00:00 2001
From: patak-js 
Date: Fri, 22 Oct 2021 07:32:07 +0200
Subject: [PATCH 2/7] fix: use ensureServingAccess

---
 packages/playground/fs-serve/__tests__/fs-serve.spec.ts | 2 +-
 packages/vite/src/node/server/middlewares/static.ts     | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
index 49d0ffd0d57bf7..33dc03858423a6 100644
--- a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
+++ b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
@@ -25,7 +25,7 @@ describe('main', () => {
 
     test('unsafe fetch', async () => {
       expect(await page.textContent('.unsafe-fetch')).toBe('')
-      expect(await page.textContent('.unsafe-fetch-status')).toBe('404') // TODO: should be 403
+      expect(await page.textContent('.unsafe-fetch-status')).toBe('403')
     })
 
     test('safe fs fetch', async () => {
diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index 0ad68870de2cb3..7e6a46fbf1c677 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -50,7 +50,7 @@ export function serveStaticMiddleware(
   const serve = sirv(dir, sirvOptions)
 
   // Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
-  return async function viteServeStaticMiddleware(req, res, next) {
+  return function viteServeStaticMiddleware(req, res, next) {
     // only serve the file if it's not an html request
     // so that html requests can fallthrough to our html middleware for
     // special processing
@@ -83,11 +83,8 @@ export function serveStaticMiddleware(
 
     const resolvedUrl = redirected || url
     const fileUrl = path.resolve(dir, resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl)
-    // TODO: should use ensureServingAccess(fileUrl, server)
-    if (!isFileServingAllowed(fileUrl, server)) {
-      return next()
-    }
-
+    ensureServingAccess(fileUrl, server)
+    
     if (redirected) {
       req.url = redirected
     }

From d2ef1bc0e557f631c846e150e7d7ad361280caea Mon Sep 17 00:00:00 2001
From: patak-js 
Date: Fri, 22 Oct 2021 07:53:58 +0200
Subject: [PATCH 3/7] fix: refactor 403

---
 .../src/node/server/middlewares/static.ts     | 25 ++++++++++++-------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index 7e6a46fbf1c677..ab9ecc10ecfc61 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -12,7 +12,7 @@ import {
   isWindows,
   slash
 } from '../../utils'
-import { AccessRestrictedError } from './error'
+import { AccessRestrictedError, renderErrorHTML } from './error'
 
 const sirvOptions: Options = {
   dev: true,
@@ -83,8 +83,13 @@ export function serveStaticMiddleware(
 
     const resolvedUrl = redirected || url
     const fileUrl = path.resolve(dir, resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl)
-    ensureServingAccess(fileUrl, server)
-    
+    if (!isFileServingAllowed(fileUrl, server)) {
+      res.statusCode = 403
+      res.write(renderErrorHTML(fsServeErrorMessage(fileUrl, server)))
+      res.end()
+      return
+    }
+
     if (redirected) {
       req.url = redirected
     }
@@ -146,15 +151,17 @@ export function isFileServingAllowed(
   return false
 }
 
-export function ensureServingAccess(url: string, server: ViteDevServer): void {
-  if (!isFileServingAllowed(url, server)) {
-    const allow = server.config.server.fs.allow
-    throw new AccessRestrictedError(
-      `The request url "${url}" is outside of Vite serving allow list:
+function fsServeErrorMessage(url: string, server: ViteDevServer) {
+  const allow = server.config.server.fs.allow
+  return `The request url "${url}" is outside of Vite serving allow list:
 
 ${allow.map((i) => `- ${i}`).join('\n')}
 
 Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`
-    )
+}
+
+export function ensureServingAccess(url: string, server: ViteDevServer): void {
+  if (!isFileServingAllowed(url, server)) {
+    throw new AccessRestrictedError(fsServeErrorMessage(url, server))
   }
 }

From 16cf302dc668f1c06d2af738336845d2e0c1954e Mon Sep 17 00:00:00 2001
From: Matias Capeletto 
Date: Fri, 22 Oct 2021 08:38:42 +0200
Subject: [PATCH 4/7] fix: revert to workaround

---
 .../fs-serve/__tests__/fs-serve.spec.ts       |  2 +-
 .../src/node/server/middlewares/static.ts     | 23 ++++++++-----------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
index 33dc03858423a6..49d0ffd0d57bf7 100644
--- a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
+++ b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
@@ -25,7 +25,7 @@ describe('main', () => {
 
     test('unsafe fetch', async () => {
       expect(await page.textContent('.unsafe-fetch')).toBe('')
-      expect(await page.textContent('.unsafe-fetch-status')).toBe('403')
+      expect(await page.textContent('.unsafe-fetch-status')).toBe('404') // TODO: should be 403
     })
 
     test('safe fs fetch', async () => {
diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index ab9ecc10ecfc61..9f21a5d5d47f90 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -82,12 +82,13 @@ export function serveStaticMiddleware(
     }
 
     const resolvedUrl = redirected || url
-    const fileUrl = path.resolve(dir, resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl)
+    const fileUrl = path.resolve(
+      dir,
+      resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl
+    )
+    // TODO: should use ensureServingAccess(fileUrl, server), so we get a 403
     if (!isFileServingAllowed(fileUrl, server)) {
-      res.statusCode = 403
-      res.write(renderErrorHTML(fsServeErrorMessage(fileUrl, server)))
-      res.end()
-      return
+      return next()
     }
 
     if (redirected) {
@@ -151,17 +152,13 @@ export function isFileServingAllowed(
   return false
 }
 
-function fsServeErrorMessage(url: string, server: ViteDevServer) {
+export function ensureServingAccess(url: string, server: ViteDevServer): void {
   const allow = server.config.server.fs.allow
-  return `The request url "${url}" is outside of Vite serving allow list:
+  if (!isFileServingAllowed(url, server)) {
+    throw new AccessRestrictedError(`The request url "${url}" is outside of Vite serving allow list:
 
 ${allow.map((i) => `- ${i}`).join('\n')}
 
-Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`
-}
-
-export function ensureServingAccess(url: string, server: ViteDevServer): void {
-  if (!isFileServingAllowed(url, server)) {
-    throw new AccessRestrictedError(fsServeErrorMessage(url, server))
+Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`)
   }
 }

From e1f47b93d16bca2b162de48aabec289ca81e24c9 Mon Sep 17 00:00:00 2001
From: Matias Capeletto 
Date: Fri, 22 Oct 2021 08:46:49 +0200
Subject: [PATCH 5/7] fix: import

---
 packages/vite/src/node/server/middlewares/static.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index 9f21a5d5d47f90..f3dc8fe6fc3820 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -12,7 +12,7 @@ import {
   isWindows,
   slash
 } from '../../utils'
-import { AccessRestrictedError, renderErrorHTML } from './error'
+import { AccessRestrictedError } from './error'
 
 const sirvOptions: Options = {
   dev: true,

From 4afa16debe414e3d2dffad67a9d41437e8e8f2fb Mon Sep 17 00:00:00 2001
From: patak-js 
Date: Fri, 22 Oct 2021 12:22:54 +0200
Subject: [PATCH 6/7] fix: ensure working

---
 .../playground/fs-serve/__tests__/fs-serve.spec.ts  |  4 ++--
 packages/vite/src/node/server/middlewares/static.ts | 13 +++++--------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
index 49d0ffd0d57bf7..c3d8ee9a9bf911 100644
--- a/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
+++ b/packages/playground/fs-serve/__tests__/fs-serve.spec.ts
@@ -24,8 +24,8 @@ describe('main', () => {
     })
 
     test('unsafe fetch', async () => {
-      expect(await page.textContent('.unsafe-fetch')).toBe('')
-      expect(await page.textContent('.unsafe-fetch-status')).toBe('404') // TODO: should be 403
+      expect(await page.textContent('.unsafe-fetch')).toMatch('403 Restricted')
+      expect(await page.textContent('.unsafe-fetch-status')).toBe('403')
     })
 
     test('safe fs fetch', async () => {
diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index f3dc8fe6fc3820..5a44a3d5303c7a 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -82,15 +82,12 @@ export function serveStaticMiddleware(
     }
 
     const resolvedUrl = redirected || url
-    const fileUrl = path.resolve(
-      dir,
-      resolvedUrl.startsWith('/') ? resolvedUrl.slice(1) : resolvedUrl
-    )
-    // TODO: should use ensureServingAccess(fileUrl, server), so we get a 403
-    if (!isFileServingAllowed(fileUrl, server)) {
-      return next()
+    let fileUrl = path.resolve(dir, resolvedUrl.replace(/^\//, ''))
+    if (resolvedUrl.endsWith('/') && !fileUrl.endsWith('/')) {
+      fileUrl = fileUrl + '/'
     }
-
+    ensureServingAccess(fileUrl, server)
+    
     if (redirected) {
       req.url = redirected
     }

From a8ec15799064e5cf5126e7f37078cfa0f2cab942 Mon Sep 17 00:00:00 2001
From: patak-js 
Date: Sat, 23 Oct 2021 08:28:05 +0200
Subject: [PATCH 7/7] chore: spacing

---
 packages/vite/src/node/server/middlewares/static.ts | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
index 5a44a3d5303c7a..4132b0d39fe3a3 100644
--- a/packages/vite/src/node/server/middlewares/static.ts
+++ b/packages/vite/src/node/server/middlewares/static.ts
@@ -150,12 +150,14 @@ export function isFileServingAllowed(
 }
 
 export function ensureServingAccess(url: string, server: ViteDevServer): void {
-  const allow = server.config.server.fs.allow
   if (!isFileServingAllowed(url, server)) {
-    throw new AccessRestrictedError(`The request url "${url}" is outside of Vite serving allow list:
+    const allow = server.config.server.fs.allow
+    throw new AccessRestrictedError(
+      `The request url "${url}" is outside of Vite serving allow list:
 
 ${allow.map((i) => `- ${i}`).join('\n')}
 
-Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`)
+Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`
+    )
   }
 }