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: 4 additions & 2 deletions packages/vite/src/node/plugins/importAnalysis.ts
Expand Up @@ -556,8 +556,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
}
}

// record as safe modules
server?.moduleGraph.safeModulesPath.add(fsPathFromUrl(url))
// record as safe modules # stripBase: https://github.com/vitejs/vite/issues/9438#issuecomment-1486662486
Copy link
Collaborator

@benmccann benmccann Jul 7, 2023

Choose a reason for hiding this comment

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

it we're going to link to a comment it should probably be #9438 (comment), but it'd be nicer to just explain why it's necessary here rather than making someone copy paste a URL into their browser to read the explanation (same below)

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 tried to fix it. Thanks for guiding me on this PR.

server?.moduleGraph.safeModulesPath.add(
fsPathFromUrl(stripBase(url, base)),
)

if (url !== specifier) {
let rewriteDone = false
Expand Down
4 changes: 3 additions & 1 deletion packages/vite/src/node/server/middlewares/static.ts
Expand Up @@ -19,6 +19,7 @@ import {
removeLeadingSlash,
shouldServeFile,
slash,
stripBase,
} from '../../utils'

const knownJavascriptExtensionRE = /\.[tj]sx?$/
Expand Down Expand Up @@ -194,7 +195,8 @@ export function isFileServingAllowed(
): boolean {
if (!server.config.server.fs.strict) return true

const file = fsPathFromUrl(url)
// stripBase: https://github.com/vitejs/vite/issues/9438#issuecomment-1486662486
const file = fsPathFromUrl(stripBase(url, server.config.rawBase))
Copy link
Member

Choose a reason for hiding this comment

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

This function is called from serveRawFsMiddleware/serveStaticMiddleware/transformMiddleware that runs after baseMiddleware (this middleware strips the base). So I think we shouldn't run stripBase here.

// base
if (config.base !== '/') {
middlewares.use(baseMiddleware(server))
}
// open in editor support
middlewares.use('/__open-in-editor', launchEditorMiddleware())
// ping request handler
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
middlewares.use(function viteHMRPingMiddleware(req, res, next) {
if (req.headers['accept'] === 'text/x-vite-ping') {
res.writeHead(204).end()
} else {
next()
}
})
// serve static files under /public
// this applies before the transform middleware so that these files are served
// as-is without transforms.
if (config.publicDir) {
middlewares.use(
servePublicMiddleware(config.publicDir, config.server.headers),
)
}
// main transform middleware
middlewares.use(transformMiddleware(server))
// serve static files
middlewares.use(serveRawFsMiddleware(server))
middlewares.use(serveStaticMiddleware(root, server))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that's how it is. I remove stripBase here


if (server._fsDenyGlob(file)) return 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