fix: allowed files logic (fix #5416) (#5420)

vitejs · Oct 26, 2021 · 414bc45 · 414bc45
1 parent 8556ffe
commit 414bc45
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 60 deletions.
diff --git a/packages/vite/src/node/plugins/resolve.ts b/packages/vite/src/node/plugins/resolve.ts
@@ -24,7 +24,8 @@ import {
   isDataUrl,
   cleanUrl,
   slash,
-  nestedResolveFrom
+  nestedResolveFrom,
+  isFileReadable
 } from '../utils'
 import { ViteDevServer, SSROptions } from '..'
 import { createFilter } from '@rollup/pluginutils'
@@ -372,15 +373,10 @@ function tryResolveFile(
   tryPrefix?: string,
   skipPackageJson?: boolean
 ): string | undefined {
-  let isReadable = false
-  try {
-    // #2051 if we don't have read permission on a directory, existsSync() still
-    // works and will result in massively slow subsequent checks (which are
-    // unnecessary in the first place)
-    fs.accessSync(file, fs.constants.R_OK)
-    isReadable = true
-  } catch (e) {}
-  if (isReadable) {
+  // #2051 if we don't have read permission on a directory, existsSync() still
+  // works and will result in massively slow subsequent checks (which are
+  // unnecessary in the first place)
+  if (isFileReadable(file)) {
     if (!fs.statSync(file).isDirectory()) {
       return getRealPath(file, preserveSymlinks) + postfix
     } else if (tryIndex) {

diff --git a/packages/vite/src/node/server/middlewares/error.ts b/packages/vite/src/node/server/middlewares/error.ts
@@ -68,35 +68,8 @@ export function errorMiddleware(
     if (allowNext) {
       next()
     } else {
-      if (err instanceof AccessRestrictedError) {
-        res.statusCode = 403
-        res.write(renderErrorHTML(err.message))
-        res.end()
-      }
       res.statusCode = 500
       res.end()
     }
   }
 }
-
-export class AccessRestrictedError extends Error {
-  constructor(msg: string) {
-    super(msg)
-  }
-}
-
-export function renderErrorHTML(msg: string): string {
-  // to have syntax highlighting and autocompletion in IDE
-  const html = String.raw
-  return html`
-    <body>
-      <h1>403 Restricted</h1>
-      <p>${msg.replace(/\n/g, '<br/>')}</p>
-      <style>
-        body {
-          padding: 1em 2em;
-        }
-      </style>
-    </body>
-  `
-}
diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts
@@ -1,4 +1,5 @@
 import path from 'path'
+import { ServerResponse } from 'http'
 import sirv, { Options } from 'sirv'
 import { Connect } from 'types/connect'
 import { normalizePath, ViteDevServer } from '../..'
@@ -10,9 +11,9 @@ import {
   isImportRequest,
   isInternalRequest,
   isWindows,
-  slash
+  slash,
+  isFileReadable
 } from '../../utils'
-import { AccessRestrictedError } from './error'
 
 const sirvOptions: Options = {
   dev: true,
@@ -51,12 +52,14 @@ export function serveStaticMiddleware(
 
   // Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
   return function viteServeStaticMiddleware(req, res, next) {
-    // only serve the file if it's not an html request
+    // only serve the file if it's not an html request or ends with `/`
     // so that html requests can fallthrough to our html middleware for
     // special processing
     // also skip internal requests `/@fs/ /@vite-client` etc...
+    const cleanedUrl = cleanUrl(req.url!)
     if (
-      path.extname(cleanUrl(req.url!)) === '.html' ||
+      cleanedUrl.endsWith('/') ||
+      path.extname(cleanedUrl) === '.html' ||
       isInternalRequest(req.url!)
     ) {
       return next()
@@ -86,8 +89,10 @@ export function serveStaticMiddleware(
     if (resolvedUrl.endsWith('/') && !fileUrl.endsWith('/')) {
       fileUrl = fileUrl + '/'
     }
-    ensureServingAccess(fileUrl, server)
-
+    if (!ensureServingAccess(fileUrl, server, res, next)) {
+      return
+    }
+
     if (redirected) {
       req.url = redirected
     }
@@ -110,7 +115,10 @@ export function serveRawFsMiddleware(
     // searching based from fs root.
     if (url.startsWith(FS_PREFIX)) {
       // restrict files outside of `fs.allow`
-      ensureServingAccess(slash(path.resolve(fsPathFromId(url))), server)
+      if (!ensureServingAccess(slash(path.resolve(fsPathFromId(url))), server, res, next)) {
+        return
+      }
+
       url = url.slice(FS_PREFIX.length)
       if (isWindows) url = url.replace(/^[A-Z]:/i, '')
 
@@ -129,35 +137,71 @@ export function isFileServingAllowed(
   // explicitly disabled
   if (server.config.server.fs.strict === false) return true
 
-  const file = ensureLeadingSlash(normalizePath(cleanUrl(url)))
+  const cleanedUrl = cleanUrl(url)
+  const file = ensureLeadingSlash(normalizePath(cleanedUrl))
 
   if (server.moduleGraph.safeModulesPath.has(file)) return true
 
   if (server.config.server.fs.allow.some((i) => file.startsWith(i + '/')))
     return true
 
   if (!server.config.server.fs.strict) {
-    server.config.logger.warnOnce(`Unrestricted file system access to "${url}"`)
-    server.config.logger.warnOnce(
-      `For security concerns, accessing files outside of serving allow list will ` +
+    if (isFileReadable(cleanedUrl)) {
+      server.config.logger.warnOnce(`Unrestricted file system access to "${url}"`)
+      server.config.logger.warnOnce(
+        `For security concerns, accessing files outside of serving allow list will ` +
         `be restricted by default in the future version of Vite. ` +
         `Refer to https://vitejs.dev/config/#server-fs-allow for more details.`
-    )
+      )
+    }
     return true
   }
 
   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:
-
-${allow.map((i) => `- ${i}`).join('\n')}
+function ensureServingAccess(
+  url: string,
+  server: ViteDevServer,
+  res: ServerResponse,
+  next: Connect.NextFunction,
+): boolean {
+  if (isFileServingAllowed(url, server)) {
+    return true
+  }
+  if (isFileReadable(cleanUrl(url))) {
+    const urlMessage = `The request url "${url}" is outside of Vite serving allow list.`
+    const hintMessage = `
+${server.config.server.fs.allow.map((i) => `- ${i}`).join('\n')}
 
 Refer to docs https://vitejs.dev/config/#server-fs-allow for configurations and more details.`
-    )
+
+    server.config.logger.error(urlMessage)
+    server.config.logger.warnOnce(hintMessage + '\n')
+    res.statusCode = 403
+    res.write(renderRestrictedErrorHTML(urlMessage + '\n' + hintMessage))
+    res.end()
+  }
+  else {
+    // if the file doesn't exist, we shouldn't restrict this path as it can
+    // be an API call. Middlewares would issue a 404 if the file isn't handled
+    next()
   }
+  return false
+}
+
+function renderRestrictedErrorHTML(msg: string): string {
+  // to have syntax highlighting and autocompletion in IDE
+  const html = String.raw
+  return html`
+    <body>
+      <h1>403 Restricted</h1>
+      <p>${msg.replace(/\n/g, '<br/>')}</p>
+      <style>
+        body {
+          padding: 1em 2em;
+        }
+      </style>
+    </body>
+  `
 }
diff --git a/packages/vite/src/node/server/searchRoot.ts b/packages/vite/src/node/server/searchRoot.ts
@@ -1,6 +1,7 @@
 import fs from 'fs'
 import { dirname } from 'path'
 import { join } from 'path'
+import { isFileReadable } from '../utils'
 
 // https://github.com/vitejs/vite/issues/2820#issuecomment-812495079
 const ROOT_FILES = [
@@ -21,9 +22,7 @@ const ROOT_FILES = [
 // yarn: https://classic.yarnpkg.com/en/docs/workspaces/#toc-how-to-use-it
 function hasWorkspacePackageJSON(root: string): boolean {
   const path = join(root, 'package.json')
-  try {
-    fs.accessSync(path, fs.constants.R_OK)
-  } catch {
+  if (!isFileReadable(path)) {
     return false
   }
   const content = JSON.parse(fs.readFileSync(path, 'utf-8')) || {}

diff --git a/packages/vite/src/node/utils.ts b/packages/vite/src/node/utils.ts
@@ -377,6 +377,21 @@ export function writeFile(
   fs.writeFileSync(filename, content)
 }
 
+/**
+ * Use instead of fs.existsSync(filename)
+ * #2051 if we don't have read permission on a directory, existsSync() still
+ * works and will result in massively slow subsequent checks (which are
+ * unnecessary in the first place)
+ */
+export function isFileReadable(filename: string): boolean {
+  try {
+    fs.accessSync(filename, fs.constants.R_OK)
+    return true
+  } catch {
+    return false
+  }
+}
+
 /**
  * Delete every file and subdirectory. **The given directory must exist.**
  * Pass an optional `skip` array to preserve files in the root directory.