Skip to content

Commit 33f96a1

Browse files
hi-ogawaOpenCode (claude-opus-4-8)
hi-ogawa
and
OpenCode (claude-opus-4-8)
authored
fix(browser): check fs access in builtin commands (#10674)
Co-authored-by: Hiroshi Ogawa <[email protected]> Co-authored-by: OpenCode (claude-opus-4-8) <[email protected]>
1 parent 60c9990 commit 33f96a1

17 files changed

Lines changed: 212 additions & 33 deletions

File tree

‎packages/browser-playwright/src/commands/screenshot.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { SerializedLocator } from '@vitest/browser'
22
import type { ScreenshotOptions } from 'vitest/browser'
33
import type { BrowserCommandContext } from 'vitest/node'
44
import { mkdir } from 'node:fs/promises'
5-
import { resolveScreenshotPath } from '@vitest/browser'
5+
import { assertBrowserApiWrite, assertBrowserFileAccess, resolveScreenshotPath } from '@vitest/browser'
66
import { dirname, normalize } from 'pathe'
77
import { getDescribedLocator } from './utils'
88

@@ -51,6 +51,9 @@ export async function takeScreenshot(
5151
if (options.save) {
5252
savePath = normalize(path)
5353

54+
assertBrowserApiWrite(context.project, savePath)
55+
assertBrowserFileAccess(context.project, savePath)
56+
5457
await mkdir(dirname(savePath), { recursive: true })
5558
}
5659

‎packages/browser-playwright/src/commands/trace.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { ParsedStack } from 'vitest'
33
import type { BrowserCommand, BrowserCommandContext, BrowserProvider } from 'vitest/node'
44
import type { PlaywrightBrowserProvider } from '../playwright'
55
import { unlink } from 'node:fs/promises'
6+
import { assertBrowserApiWrite, assertBrowserFileAccess } from '@vitest/browser'
67
import { basename, dirname, relative, resolve } from 'pathe'
78
import { getDescribedLocator } from './utils'
89

@@ -52,6 +53,8 @@ export const stopChunkTrace: BrowserCommand<[{ name: string }]> = async (
5253
) => {
5354
if (isPlaywrightProvider(context.provider)) {
5455
const path = resolveTracesPath(context, name)
56+
assertBrowserApiWrite(context.project, path)
57+
assertBrowserFileAccess(context.project, path)
5558
context.provider.pendingTraces.delete(path)
5659
await context.context.tracing.stopChunk({ path })
5760
return { tracePath: path }
@@ -163,6 +166,10 @@ export const deleteTracing: BrowserCommand<[{ traces: string[] }]> = async (
163166
throw new Error(`stopChunkTrace cannot be called outside of the test file.`)
164167
}
165168
if (isPlaywrightProvider(context.provider)) {
169+
for (const trace of traces) {
170+
assertBrowserApiWrite(context.project, trace)
171+
assertBrowserFileAccess(context.project, trace)
172+
}
166173
return Promise.all(
167174
traces.map(trace => unlink(trace).catch((err) => {
168175
if (err.code === 'ENOENT') {
@@ -184,6 +191,8 @@ export const annotateTraces: BrowserCommand<[{ traces: string[]; testId: string
184191
) => {
185192
const vitest = project.vitest
186193
await Promise.all(traces.map((trace) => {
194+
assertBrowserApiWrite(project, trace)
195+
assertBrowserFileAccess(project, trace)
187196
const entity = vitest.state.getReportedEntityById(testId)
188197
const location = entity?.location
189198
? {

‎packages/browser-playwright/src/commands/upload.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { SerializedLocator } from '@vitest/browser'
22
import type { UserEventUploadOptions } from 'vitest/browser'
33
import type { UserEventCommand } from './utils'
4+
import { assertBrowserFileAccess } from '@vitest/browser'
45
import { resolve } from 'pathe'
56
import { getDescribedLocator } from './utils'
67

@@ -22,7 +23,9 @@ export const upload: UserEventCommand<(element: SerializedLocator, files: Array<
2223

2324
const playwrightFiles = files.map((file) => {
2425
if (typeof file === 'string') {
25-
return resolve(root, file)
26+
const filepath = resolve(root, file)
27+
assertBrowserFileAccess(context.project, filepath)
28+
return filepath
2629
}
2730
return {
2831
name: file.name,

‎packages/browser/src/node/commands/fs.ts‎

Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,15 @@
11
import type { BrowserCommands } from 'vitest/browser'
2-
import type { BrowserCommand, TestProject } from 'vitest/node'
2+
import type { BrowserCommand } from 'vitest/node'
33
import fs, { promises as fsp } from 'node:fs'
44
import { basename, dirname, resolve } from 'node:path'
55
import mime from 'mime/lite'
6-
import { isFileLoadingAllowed } from 'vitest/node'
7-
import { slash } from '../utils'
8-
9-
function assertFileAccess(path: string, project: TestProject) {
10-
if (
11-
!isFileLoadingAllowed(project.vite.config, path)
12-
&& !isFileLoadingAllowed(project.vitest.vite.config, path)
13-
) {
14-
throw new Error(
15-
`Access denied to "${path}". See Vite config documentation for "server.fs": https://vitejs.dev/config/server-options.html#server-fs-strict.`,
16-
)
17-
}
18-
}
19-
20-
function assertWrite(path: string, project: TestProject) {
21-
if (!project.config.browser.api.allowWrite || !project.vitest.config.api.allowWrite) {
22-
throw new Error(`Cannot modify file "${path}". File writing is disabled because server is exposed to the internet, see https://vitest.dev/config/browser/api.`)
23-
}
24-
}
6+
import { assertBrowserApiWrite, assertBrowserFileAccess } from '../utils'
257

268
export const readFile: BrowserCommand<
279
Parameters<BrowserCommands['readFile']>
2810
> = async ({ project }, path, options = {}) => {
2911
const filepath = resolve(project.config.root, path)
30-
assertFileAccess(slash(filepath), project)
12+
assertBrowserFileAccess(project, filepath)
3113
// never return a Buffer
3214
if (typeof options === 'object' && !options.encoding) {
3315
options.encoding = 'utf-8'
@@ -38,9 +20,9 @@ export const readFile: BrowserCommand<
3820
export const writeFile: BrowserCommand<
3921
Parameters<BrowserCommands['writeFile']>
4022
> = async ({ project }, path, data, options) => {
41-
assertWrite(path, project)
23+
assertBrowserApiWrite(project, path)
4224
const filepath = resolve(project.config.root, path)
43-
assertFileAccess(slash(filepath), project)
25+
assertBrowserFileAccess(project, filepath)
4426
const dir = dirname(filepath)
4527
if (!fs.existsSync(dir)) {
4628
await fsp.mkdir(dir, { recursive: true })
@@ -51,15 +33,15 @@ export const writeFile: BrowserCommand<
5133
export const removeFile: BrowserCommand<
5234
Parameters<BrowserCommands['removeFile']>
5335
> = async ({ project }, path) => {
54-
assertWrite(path, project)
36+
assertBrowserApiWrite(project, path)
5537
const filepath = resolve(project.config.root, path)
56-
assertFileAccess(slash(filepath), project)
38+
assertBrowserFileAccess(project, filepath)
5739
await fsp.rm(filepath)
5840
}
5941

6042
export const _fileInfo: BrowserCommand<[path: string, encoding: BufferEncoding]> = async ({ project }, path, encoding) => {
6143
const filepath = resolve(project.config.root, path)
62-
assertFileAccess(slash(filepath), project)
44+
assertBrowserFileAccess(project, filepath)
6345
const content = await fsp.readFile(filepath, encoding || 'base64')
6446
return {
6547
content,

‎packages/browser/src/node/commands/screenshotMatcher/index.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
import type { SerializedLocator } from '@vitest/browser'
22
import type { SnapshotUpdateState } from 'vitest'
33
import type { ScreenshotMatcherOptions } from 'vitest/browser'
4-
import type { BrowserCommand, BrowserCommandContext } from 'vitest/node'
4+
import type { BrowserCommand, BrowserCommandContext, TestProject } from 'vitest/node'
55
import type { ScreenshotMatcherArguments, ScreenshotMatcherOutput } from '../../../shared/screenshotMatcher/types'
66
import type { AnyCodec } from './codecs'
77
import type { AnyComparator } from './comparators'
88
import type { TypedArray } from './types'
99
import type { ResolvedOptions } from './utils'
1010
import { mkdir, readFile, writeFile } from 'node:fs/promises'
1111
import { basename, dirname } from 'pathe'
12+
import { assertBrowserApiWrite, assertBrowserFileAccess } from '../../utils'
1213
import { asyncTimeout, resolveOptions, takeDecodedScreenshot, takeScreenshotBuffer } from './utils'
1314

1415
/** Decoded image data with dimensions metadata. */
@@ -148,7 +149,7 @@ export const screenshotMatcher: BrowserCommand<ScreenshotMatcherArguments> = asy
148149
comparatorOptions,
149150
})
150151

151-
await performSideEffects(outcome, codec)
152+
await performSideEffects(outcome, codec, context.project)
152153

153154
return buildOutput(outcome, timeout)
154155
}
@@ -276,13 +277,15 @@ async function determineOutcome(
276277
async function performSideEffects(
277278
outcome: MatchOutcome,
278279
codec: AnyCodec,
280+
project: TestProject,
279281
): Promise<void> {
280282
switch (outcome.type) {
281283
case 'missing-reference':
282284
case 'update-reference': {
283285
await writeScreenshot(
284286
outcome.reference.path,
285287
await encodeScreenshot(outcome.reference, codec),
288+
project,
286289
)
287290

288291
break
@@ -292,12 +295,14 @@ async function performSideEffects(
292295
await writeScreenshot(
293296
outcome.actual.path,
294297
await encodeScreenshot(outcome.actual, codec),
298+
project,
295299
)
296300

297301
if (outcome.diff) {
298302
await writeScreenshot(
299303
outcome.diff.path,
300304
await codec.encode(outcome.diff.image, {}),
305+
project,
301306
)
302307
}
303308

@@ -555,8 +560,10 @@ async function takeScreenshotData({
555560
}
556561

557562
/** Writes encoded images to disk, creating parent directories as needed. */
558-
async function writeScreenshot(path: string, image: TypedArray) {
563+
async function writeScreenshot(path: string, image: TypedArray, project: TestProject) {
559564
try {
565+
assertBrowserApiWrite(project, path)
566+
assertBrowserFileAccess(project, path)
560567
await mkdir(dirname(path), { recursive: true })
561568
await writeFile(path, image)
562569
}

‎packages/browser/src/node/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ export function defineBrowserCommand<T extends unknown[]>(
2323
}
2424

2525
// export type { ProjectBrowser } from './project'
26-
export { parseKeyDef, resolveScreenshotPath } from './utils'
26+
export { assertBrowserApiWrite, assertBrowserFileAccess, parseKeyDef, resolveScreenshotPath } from './utils'
2727

2828
export const createBrowserServer: BrowserServerFactory = async (options) => {
2929
const project = options.project

‎packages/browser/src/node/rpc.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { ServerMockResolver } from '@vitest/mocker/node'
1212
import { extractSourcemapFromFile } from '@vitest/utils/source-map/node'
1313
import { createBirpc } from 'birpc'
1414
import { parse, stringify } from 'flatted'
15-
import { dirname, join } from 'pathe'
15+
import { dirname, join, resolve } from 'pathe'
1616
import { createDebugger, isFileLoadingAllowed, isValidApiRequest } from 'vitest/node'
1717
import { WebSocketServer } from 'ws'
1818

@@ -211,6 +211,19 @@ export function setupBrowserRpc(globalServer: ParentBrowserProject, defaultMocke
211211
)
212212
}
213213
}
214+
else {
215+
// attachment files are copied into `attachmentsDir`, so confine
216+
// client-supplied paths to Vite's `server.fs` boundary
217+
const attachments = artifact.type === 'internal:annotation'
218+
? (artifact.annotation.attachment ? [artifact.annotation.attachment] : [])
219+
: (artifact.attachments ?? [])
220+
for (const attachment of attachments) {
221+
const path = attachment.path
222+
if (path && !path.startsWith('http://') && !path.startsWith('https://')) {
223+
checkFileAccess(resolve(project.config.root, path))
224+
}
225+
}
226+
}
214227

215228
return vitest._testRun.recordArtifact(id, artifact)
216229
},

‎packages/browser/src/node/utils.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
import { defaultKeyMap } from '@testing-library/user-event/dist/esm/keyboard/keyMap.js'
99
import { parseKeyDef as tlParse } from '@testing-library/user-event/dist/esm/keyboard/parseKeyDef.js'
1010
import { basename, dirname, relative, resolve } from 'pathe'
11+
import { isFileLoadingAllowed } from 'vitest/node'
1112

1213
declare enum DOM_KEY_LOCATION {
1314
STANDARD = 0,
@@ -95,3 +96,23 @@ export async function getBrowserProvider(
9596
export function slash(path: string): string {
9697
return path.replace(/\\/g, '/').replace(/\/+/g, '/')
9798
}
99+
100+
export function assertBrowserFileAccess(project: TestProject, path: string): void {
101+
const normalized = slash(path)
102+
if (
103+
!isFileLoadingAllowed(project.vite.config, normalized)
104+
&& !isFileLoadingAllowed(project.vitest.vite.config, normalized)
105+
) {
106+
throw new Error(
107+
`Access denied to "${path}". See Vite config documentation for "server.fs": https://vitejs.dev/config/server-options.html#server-fs-strict.`,
108+
)
109+
}
110+
}
111+
112+
export function assertBrowserApiWrite(project: TestProject, path: string): void {
113+
if (!project.config.browser.api.allowWrite || !project.vitest.config.api.allowWrite) {
114+
throw new Error(
115+
`Cannot modify file "${path}". File writing is disabled because the server is exposed to the internet, see https://vitest.dev/config/browser/api.`,
116+
)
117+
}
118+
}

‎test/browser/fixtures/command-permissions-screenshot-denied/screenshot-denied.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import { test } from 'vitest'
2+
import { page } from 'vitest/browser'
3+
4+
test('screenshot denied path', async () => {
5+
// write is allowed, but the target is denied via `server.fs.deny`
6+
await page.screenshot({ path: 'my-secret.png' })
7+
})

‎test/browser/fixtures/command-permissions-screenshot-denied/vitest.config.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { fileURLToPath } from 'node:url'
2+
import { defineConfig } from 'vitest/config'
3+
import { instances, provider } from '../../settings'
4+
5+
export default defineConfig({
6+
cacheDir: fileURLToPath(new URL('./node_modules/.vite', import.meta.url)),
7+
server: {
8+
fs: {
9+
deny: ['my-secret.png'],
10+
},
11+
},
12+
test: {
13+
browser: {
14+
enabled: true,
15+
provider,
16+
instances,
17+
headless: true,
18+
screenshotFailures: false,
19+
},
20+
},
21+
})

0 commit comments

Comments
 (0)