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

fix(importAnalysis): strip url base before passing as safeModulePaths #13712

Merged
merged 10 commits into from Jul 29, 2023
6 changes: 5 additions & 1 deletion packages/vite/src/node/plugins/importAnalysis.ts
Expand Up @@ -557,7 +557,11 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
}

// record as safe modules
server?.moduleGraph.safeModulesPath.add(fsPathFromUrl(url))
// when base is set, safeModulesPath should not has base prefix inside.
xinxinhe1810 marked this conversation as resolved.
Show resolved Hide resolved
// See https://github.com/vitejs/vite/issues/9438#issuecomment-1465270409
server?.moduleGraph.safeModulesPath.add(
fsPathFromUrl(stripBase(url, base)),
)

if (url !== specifier) {
let rewriteDone = false
Expand Down
104 changes: 104 additions & 0 deletions playground/fs-serve/__tests__/base/fs-serve-base.spec.ts
@@ -0,0 +1,104 @@
import fetch from 'node-fetch'
import { beforeAll, describe, expect, test } from 'vitest'
import testJSON from '../../safe.json'
import { isServe, page, viteTestUrl } from '~utils'

const stringified = JSON.stringify(testJSON)

describe.runIf(isServe)('main', () => {
beforeAll(async () => {
const srcPrefix = viteTestUrl.endsWith('/') ? '' : '/'
await page.goto(viteTestUrl + srcPrefix + 'src/')
})

test('default import', async () => {
expect(await page.textContent('.full')).toBe(stringified)
})

test('named import', async () => {
expect(await page.textContent('.named')).toBe(testJSON.msg)
})

test('safe fetch', async () => {
expect(await page.textContent('.safe-fetch')).toMatch('KEY=safe')
expect(await page.textContent('.safe-fetch-status')).toBe('200')
})

test('safe fetch with query', async () => {
expect(await page.textContent('.safe-fetch-query')).toMatch('KEY=safe')
expect(await page.textContent('.safe-fetch-query-status')).toBe('200')
})

test('safe fetch with special characters', async () => {
expect(
await page.textContent('.safe-fetch-subdir-special-characters'),
).toMatch('KEY=safe')
expect(
await page.textContent('.safe-fetch-subdir-special-characters-status'),
).toBe('200')
})

test('unsafe fetch', async () => {
expect(await page.textContent('.unsafe-fetch')).toMatch('403 Restricted')
expect(await page.textContent('.unsafe-fetch-status')).toBe('403')
})

test('unsafe fetch with special characters (#8498)', async () => {
expect(await page.textContent('.unsafe-fetch-8498')).toBe('')
expect(await page.textContent('.unsafe-fetch-8498-status')).toBe('404')
})

test('unsafe fetch with special characters 2 (#8498)', async () => {
expect(await page.textContent('.unsafe-fetch-8498-2')).toBe('')
expect(await page.textContent('.unsafe-fetch-8498-2-status')).toBe('404')
})

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('safe fs fetch', async () => {
expect(await page.textContent('.safe-fs-fetch-query')).toBe(stringified)
expect(await page.textContent('.safe-fs-fetch-query-status')).toBe('200')
})

test('safe fs fetch with special characters', async () => {
expect(await page.textContent('.safe-fs-fetch-special-characters')).toBe(
stringified,
)
expect(
await page.textContent('.safe-fs-fetch-special-characters-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('unsafe fs fetch with special characters (#8498)', async () => {
expect(await page.textContent('.unsafe-fs-fetch-8498')).toBe('')
expect(await page.textContent('.unsafe-fs-fetch-8498-status')).toBe('404')
})

test('unsafe fs fetch with special characters 2 (#8498)', async () => {
expect(await page.textContent('.unsafe-fs-fetch-8498-2')).toBe('')
expect(await page.textContent('.unsafe-fs-fetch-8498-2-status')).toBe('404')
})

test('nested entry', async () => {
expect(await page.textContent('.nested-entry')).toBe('foobar')
})

test('denied', async () => {
expect(await page.textContent('.unsafe-dotenv')).toBe('404')
})
})

describe('fetch', () => {
test('serve with configured headers', async () => {
const res = await fetch(viteTestUrl + '/src/')
expect(res.headers.get('x-served-by')).toBe('vite')
})
})
59 changes: 59 additions & 0 deletions playground/fs-serve/__tests__/base/serve.ts
@@ -0,0 +1,59 @@
import path from 'node:path'
import { mergeConfig } from 'vite'
import config from '../../root/vite.config-base'
import { isBuild, page, rootDir, setViteUrl, viteTestUrl } from '~utils'

export async function serve(): Promise<{ close(): Promise<void> }> {
const { createServer } = await import('vite')
process.env.VITE_INLINE = 'inline-serve'

const options = {
...config,
root: rootDir,
logLevel: 'silent',
build: {
target: 'esnext',
},
server: {
watch: {
usePolling: true,
interval: 100,
},
host: true,
fs: {
strict: !isBuild,
},
},
}

const rewriteTestRootOptions = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot directly use the configuration from vite.config-base.js here. In vite.config-base.js, the variable __dirname is used, which returns a value based on "playground" instead of "playground-temp" that is used for testing. I guess this may be a bug. That's why I am using "serve" to start the testing server instead of relying on vite.config-base.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image image

The config file read in __tests__ from vite.config.js is different from the one read in playground-temp, especially when it involves expressions like __dirname.
like:

{
  build: {
    rollupOptions: {
      input: {
        main: path.resolve(__dirname, 'src/index.html'),
      },
    },
  },
  server: {
    fs: {
      strict: true,
      allow: [path.resolve(__dirname, 'src')],
    },
}

Copy link
Member

Choose a reason for hiding this comment

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

This is strange, we are using __dirname in the worker and assets playgrounds too in the configs. Maybe there is an issue here because the root is set?

Copy link
Member

Choose a reason for hiding this comment

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

You were correct, I sent a PR to fix the config variants handling here:

build: {
rollupOptions: {
input: {
main: path.resolve(rootDir, 'src/index.html'),
},
},
},
server: {
fs: {
strict: true,
allow: [path.resolve(rootDir, 'src')],
},
},
define: {
ROOT: JSON.stringify(path.dirname(rootDir).replace(/\\/g, '/')),
},
}

const viteServer = await (
await createServer(mergeConfig(options, rewriteTestRootOptions))
).listen()

// use resolved port/base from server
const devBase = viteServer.config.base === '/' ? '' : viteServer.config.base

setViteUrl(`http://localhost:${viteServer.config.server.port}${devBase}`)
await page.goto(viteTestUrl)

return viteServer
}
11 changes: 5 additions & 6 deletions playground/fs-serve/__tests__/fs-serve.spec.ts
Expand Up @@ -7,7 +7,8 @@ const stringified = JSON.stringify(testJSON)

describe.runIf(isServe)('main', () => {
beforeAll(async () => {
await page.goto(viteTestUrl + '/src/')
const srcPrefix = viteTestUrl.endsWith('/') ? '' : '/'
await page.goto(viteTestUrl + srcPrefix + 'src/')
})

test('default import', async () => {
Expand Down Expand Up @@ -66,7 +67,9 @@ describe.runIf(isServe)('main', () => {
expect(await page.textContent('.safe-fs-fetch-special-characters')).toBe(
stringified,
)
expect(await page.textContent('.safe-fs-fetch-status')).toBe('200')
expect(
await page.textContent('.safe-fs-fetch-special-characters-status'),
).toBe('200')
})

test('unsafe fs fetch', async () => {
Expand All @@ -88,10 +91,6 @@ describe.runIf(isServe)('main', () => {
expect(await page.textContent('.nested-entry')).toBe('foobar')
})

test('nested entry', async () => {
xinxinhe1810 marked this conversation as resolved.
Show resolved Hide resolved
expect(await page.textContent('.nested-entry')).toBe('foobar')
})

test('denied', async () => {
expect(await page.textContent('.unsafe-dotenv')).toBe('404')
})
Expand Down
5 changes: 4 additions & 1 deletion playground/fs-serve/package.json
Expand Up @@ -7,6 +7,9 @@
"dev": "vite root",
"build": "vite build root",
"debug": "node --inspect-brk ../../packages/vite/bin/vite",
"preview": "vite preview root"
"preview": "vite preview root",
"dev:base": "vite root --config ./root/vite.config-base.js",
"build:base": "vite build root --config ./root/vite.config-base.js",
"preview:base": "vite preview root --config ./root/vite.config-base.js"
}
}
34 changes: 20 additions & 14 deletions playground/fs-serve/root/src/index.html
Expand Up @@ -53,8 +53,10 @@ <h2>Denied</h2>
text('.full', JSON.stringify(json))
text('.named', msg)

const base = typeof BASE !== 'undefined' ? BASE : ''
xinxinhe1810 marked this conversation as resolved.
Show resolved Hide resolved

// inside allowed dir, safe fetch
fetch('/src/safe.txt')
fetch(base + '/src/safe.txt')
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we've got /base/ then we'll end up with a //. This should probably use joinUrlSegments or path.posix.join

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 use joinUrlSegments to fix that

.then((r) => {
text('.safe-fetch-status', r.status)
return r.text()
Expand All @@ -64,7 +66,7 @@ <h2>Denied</h2>
})

// inside allowed dir with query, safe fetch
fetch('/src/safe.txt?query')
fetch(base + '/src/safe.txt?query')
.then((r) => {
text('.safe-fetch-query-status', r.status)
return r.text()
Expand All @@ -74,7 +76,7 @@ <h2>Denied</h2>
})

// inside allowed dir, safe fetch
fetch('/src/subdir/safe.txt')
fetch(base + '/src/subdir/safe.txt')
.then((r) => {
text('.safe-fetch-subdir-status', r.status)
return r.text()
Expand All @@ -84,7 +86,7 @@ <h2>Denied</h2>
})

// inside allowed dir, with special characters, safe fetch
fetch('/src/special%20characters%20%C3%A5%C3%A4%C3%B6/safe.txt')
fetch(base + '/src/special%20characters%20%C3%A5%C3%A4%C3%B6/safe.txt')
.then((r) => {
text('.safe-fetch-subdir-special-characters-status', r.status)
return r.text()
Expand All @@ -94,7 +96,7 @@ <h2>Denied</h2>
})

// outside of allowed dir, treated as unsafe
fetch('/unsafe.txt')
fetch(base + '/unsafe.txt')
.then((r) => {
text('.unsafe-fetch-status', r.status)
return r.text()
Expand All @@ -107,7 +109,7 @@ <h2>Denied</h2>
})

// outside of allowed dir with special characters #8498
fetch('/src/%2e%2e%2funsafe%2etxt')
fetch(base + '/src/%2e%2e%2funsafe%2etxt')
.then((r) => {
text('.unsafe-fetch-8498-status', r.status)
return r.text()
Expand All @@ -120,7 +122,7 @@ <h2>Denied</h2>
})

// outside of allowed dir with special characters 2 #8498
fetch('/src/%252e%252e%252funsafe%252etxt')
fetch(base + '/src/%252e%252e%252funsafe%252etxt')
.then((r) => {
text('.unsafe-fetch-8498-2-status', r.status)
return r.text()
Expand All @@ -133,7 +135,7 @@ <h2>Denied</h2>
})

// imported before, should be treated as safe
fetch('/@fs/' + ROOT + '/safe.json')
fetch(base + '/@fs/' + ROOT + '/safe.json')
.then((r) => {
text('.safe-fs-fetch-status', r.status)
return r.json()
Expand All @@ -143,7 +145,7 @@ <h2>Denied</h2>
})

// imported before with query, should be treated as safe
fetch('/@fs/' + ROOT + '/safe.json?query')
fetch(base + '/@fs/' + ROOT + '/safe.json?query')
.then((r) => {
text('.safe-fs-fetch-query-status', r.status)
return r.json()
Expand All @@ -153,7 +155,7 @@ <h2>Denied</h2>
})

// not imported before, outside of root, treated as unsafe
fetch('/@fs/' + ROOT + '/unsafe.json')
fetch(base + '/@fs/' + ROOT + '/unsafe.json')
.then((r) => {
text('.unsafe-fs-fetch-status', r.status)
return r.json()
Expand All @@ -166,7 +168,7 @@ <h2>Denied</h2>
})

// outside root with special characters #8498
fetch('/@fs/' + ROOT + '/root/src/%2e%2e%2f%2e%2e%2funsafe%2ejson')
fetch(base + '/@fs/' + ROOT + '/root/src/%2e%2e%2f%2e%2e%2funsafe%2ejson')
.then((r) => {
text('.unsafe-fs-fetch-8498-status', r.status)
return r.json()
Expand All @@ -177,7 +179,10 @@ <h2>Denied</h2>

// outside root with special characters 2 #8498
fetch(
'/@fs/' + ROOT + '/root/src/%252e%252e%252f%252e%252e%252funsafe%252ejson',
base +
'/@fs/' +
ROOT +
'/root/src/%252e%252e%252f%252e%252e%252funsafe%252ejson',
)
.then((r) => {
text('.unsafe-fs-fetch-8498-2-status', r.status)
Expand All @@ -189,7 +194,8 @@ <h2>Denied</h2>

// not imported before, inside root with special characters, treated as safe
fetch(
'/@fs/' +
base +
'/@fs/' +
ROOT +
'/root/src/special%20characters%20%C3%A5%C3%A4%C3%B6/safe.json',
)
Expand All @@ -202,7 +208,7 @@ <h2>Denied</h2>
})

// .env, denied by default
fetch('/@fs/' + ROOT + '/root/.env')
fetch(base + '/@fs/' + ROOT + '/root/.env')
.then((r) => {
text('.unsafe-dotenv', r.status)
})
Expand Down
36 changes: 36 additions & 0 deletions playground/fs-serve/root/vite.config-base.js
@@ -0,0 +1,36 @@
import path from 'node:path'
import { defineConfig } from 'vite'

const BASE = '/base/'

export default defineConfig({
base: BASE,
build: {
rollupOptions: {
input: {
main: path.resolve(__dirname, 'src/index.html'),
},
},
},
server: {
fs: {
strict: true,
allow: [path.resolve(__dirname, 'src')],
},
hmr: {
overlay: false,
},
headers: {
'x-served-by': 'vite',
},
},
preview: {
headers: {
'x-served-by': 'vite',
},
},
define: {
ROOT: JSON.stringify(path.dirname(__dirname).replace(/\\/g, '/')),
BASE: JSON.stringify(BASE),
},
})