-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Changes from 4 commits
fa92a74
4fcfcf4
7dd7d4c
f9831af
1cc1ef8
aaf7799
eb81f16
5bb2605
d075dff
4556271
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,6 +19,7 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
removeLeadingSlash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shouldServeFile, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stripBase, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from '../../utils' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const knownJavascriptExtensionRE = /\.[tj]sx?$/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is called from vite/packages/vite/src/node/server/index.ts Lines 614 to 646 in 1292ad0
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The config file read in {
build: {
rollupOptions: {
input: {
main: path.resolve(__dirname, 'src/index.html'),
},
},
},
server: {
fs: {
strict: true,
allow: [path.resolve(__dirname, 'src')],
},
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is strange, we are using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we've got There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i use |
||
.then((r) => { | ||
text('.safe-fetch-status', r.status) | ||
return r.text() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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) | ||
|
@@ -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', | ||
) | ||
|
@@ -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) | ||
}) | ||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.